Check that Placement is not null before trying to access it

[sgiacomel]

If Placement is missing after updating a service with previous placement constraints in a swarm with multiple nodes, it sends the swarm in an unrecoverable state.

closes #2947
closes moby/moby#37883

- What I did
I created a swarm with 2 nodes. I created a service using the docker api with a placement constraint. I then updated the service passing a json load with no Placement field.
moby/moby#37883 (comment)

- How I did it
I mainly used curl to issue docker api calls

- How to test it
I can provide more details

- Description for the changelog
Prevents nil pointer reference in nodeMatches when Placement is null

Signed-off-by: Simone Giacomel sgiacomel@medstack.co

[thaJeztah]

thanks! I left a comment inline

[thaJeztah]

Oh! I probably shouldn't have used the "suggest" option on Github as that creates an extra commit (without a proper DCO signed-off-by); would you be able to squash both commits, and make sure that the squashed commit has your signed-off-by as last line of the commit message ? Let me know if you need a hand

[sgiacomel]

Oh! I probably shouldn't have used the "suggest" option on Github as that creates an extra commit (without a proper DCO signed-off-by); would you be able to squash both commits, and make sure that the squashed commit has your signed-off-by as last line of the commit message ? Let me know if you need a hand

Ok, I think I straightened things out

[thaJeztah]

Hmm.. looks like there's three commits now 😂. One of those is a merge commit, and that likely doesn't have a sign-off 😅

Let me try walk you through it; first of all; if you run into trouble with any of these steps, as long as you don't git push your changes, you'll always have GitHub as a "backup". Also git rebase --abort can be your friend.

Assuming the remotes for your local fork are configured to have the upstream remote point to the docker/swarmkit repository, and origin point to your fork (sgiacomel/swarmkit);

First, make sure that your remotes are up to date with the latest changes;

git fetch upstream -v;
git fetch origin -v;

Make sure you're on the 2947-check-placement-not-null branch;

git checkout 2947-check-placement-not-null

Then, start an interactive rebase, based on the "master" branch in this repository;

git rebase -i -s upstream/master

This should open your default editor, showing a list of the commits that are being rebased; something like

pick b4bd091f Squashed commit
pick 463aea8c Fixed the logic. My bad.

# Rebase ebe39a32..463aea8c onto ebe39a32 (2 commands)

Now, to "squash" the commits, change pick on the second line to squash:

pick b4bd091f Squashed commit
squash 463aea8c Fixed the logic. My bad.

# Rebase ebe39a32..463aea8c onto ebe39a32 (2 commands)

Safe the file, and exit your editor.

Git will open your editor again, but this time with the commit messages of both commits to allow you to set the correct commit message;

# This is a combination of 2 commits.
# This is the 1st commit message:

Squashed commit

Signed-off-by: Simone Giacomel <sgiacomel@medstack.co>

# This is the commit message #2:

Fixed the logic. My bad.

Signed-off-by: Simone Giacomel <sgiacomel@medstack.co>

Edit the commit message to describe the change (and make sure that the last line has the sign-off), e.g.

Check that Placement is not null before trying to access it

If Placement is missing after updating a service with previous placement
constraints in a swarm with multiple nodes, it sends the swarm in an
unrecoverable state.

Signed-off-by: Simone Giacomel <sgiacomel@medstack.co>

Save the file and exit your editor;

Successfully rebased and updated refs/heads/2947-check-placement-not-null.

Now you need to force push to update the PR

git push --force origin 

After that, the PR here should automatically update, and there should be a single commit in the PR 👍

Hope this helps!

[sgiacomel]

Hmm.. looks like there's three commits now joy. One of those is a merge commit, and that likely doesn't have a sign-off sweat_smile

Let me try walk you through it; first of all; if you run into trouble with any of these steps, as long as you don't git push your changes, you'll always have GitHub as a "backup". Also git rebase --abort can be your friend.

Assuming the remotes for your local fork are configured to have the upstream remote point to the docker/swarmkit repository, and origin point to your fork (sgiacomel/swarmkit);

First, make sure that your remotes are up to date with the latest changes;

git fetch upstream -v;
git fetch origin -v;

Make sure you're on the 2947-check-placement-not-null branch;

git checkout 2947-check-placement-not-null

Then, start an interactive rebase, based on the "master" branch in this repository;

git rebase -i -s upstream/master

This should open your default editor, showing a list of the commits that are being rebased; something like

pick b4bd091f Squashed commit
pick 463aea8c Fixed the logic. My bad.

# Rebase ebe39a32..463aea8c onto ebe39a32 (2 commands)

Now, to "squash" the commits, change pick on the second line to squash:

pick b4bd091f Squashed commit
squash 463aea8c Fixed the logic. My bad.

# Rebase ebe39a32..463aea8c onto ebe39a32 (2 commands)

Safe the file, and exit your editor.

Git will open your editor again, but this time with the commit messages of both commits to allow you to set the correct commit message;

# This is a combination of 2 commits.
# This is the 1st commit message:

Squashed commit

Signed-off-by: Simone Giacomel <sgiacomel@medstack.co>

# This is the commit message #2:

Fixed the logic. My bad.

Signed-off-by: Simone Giacomel <sgiacomel@medstack.co>

Edit the commit message to describe the change (and make sure that the last line has the sign-off), e.g.

Check that Placement is not null before trying to access it

If Placement is missing after updating a service with previous placement
constraints in a swarm with multiple nodes, it sends the swarm in an
unrecoverable state.

Signed-off-by: Simone Giacomel <sgiacomel@medstack.co>

Save the file and exit your editor;

Successfully rebased and updated refs/heads/2947-check-placement-not-null.

Now you need to force push to update the PR

git push --force origin 

After that, the PR here should automatically update, and there should be a single commit in the PR +1

Hope this helps!

Thanks for your help, I clearly need to learn git more!
I followed your steps but something is quite different with my local setup since it looks like I lost the initial commit too (git didn't prompt me with another editor after the rebase and I got this error:

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply b4bd091... Squashed commit

So after I pushed, this PR was closed automatically since there were no differences!

#2950

So I just thought to open a new one, but I will definitely keep playing locally to get this right the next time.