Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Openstack support for rolling-update status #7050

Merged
merged 1 commit into from
Jun 8, 2019

Conversation

drekle
Copy link
Contributor

@drekle drekle commented May 22, 2019

/sig openstack

I have incremented generation when an instance group or cluster resource has been modified. Both these generation values are stored on the metadata of openstack instances. When a rolling update is performed it compares the generation of the cluster and instance group in swift with those on the instance.

One issue with this is that a new generation does not necessarily mean that a rolling update needs to be performed, such as min and max number of nodes.

~ $ kops --name derek.k8s.local rolling-update cluster
NAME			STATUS	NEEDUPDATE	READY	MIN	MAX	NODES
bastions		Ready	0		1	1	1	0
master-cloud-rtp-1-a	Ready	0		1	1	1	1
master-cloud-rtp-1-b	Ready	0		1	1	1	1
master-cloud-rtp-1-c	Ready	0		1	1	1	1
nodes			Ready	0		9	9	9	8

No rolling-update required.
~ $ kops --name derek.k8s.local edit ig nodes
~ $ kops --name derek.k8s.local rolling-update cluster
NAME			STATUS		NEEDUPDATE	READY	MIN	MAX	NODES
bastions		Ready		0		1	1	1	0
master-cloud-rtp-1-a	Ready		0		1	1	1	1
master-cloud-rtp-1-b	Ready		0		1	1	1	1
master-cloud-rtp-1-c	Ready		0		1	1	1	1
nodes			NeedsUpdate	9		0	5	5	8

Must specify --yes to rolling-update.

This PR is intended to resolve #7056

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. area/provider/openstack Issues or PRs related to openstack provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 22, 2019
@k8s-ci-robot k8s-ci-robot requested review from rdrgmnzs and zetaab May 22, 2019 20:45
@drekle
Copy link
Contributor Author

drekle commented May 23, 2019

My last commit will do a DeepEqual comparison of cluster and instance group objects with their target object in the KOPS_STATE_STORE. Replacing a file which defines the cluster, 3 master ig's, one bastion ig, and one node ig.

$ kops --name derek.k8s.local get ig nodes -o json | jq '.metadata.generation'
3

$ kops --name derek.k8s.local replace -f ~/derek.yaml
E0523 08:41:30.300866   80930 replace.go:149] Ignoring cluster (derek.k8s.local), no changes made
E0523 08:41:31.581909   80930 replace.go:193] Ignoring instance group (bastions), no changes made
E0523 08:41:32.965615   80930 replace.go:193] Ignoring instance group (master-cloud-rtp-1-a), no changes made
E0523 08:41:34.250612   80930 replace.go:193] Ignoring instance group (master-cloud-rtp-1-b), no changes made
E0523 08:41:35.505895   80930 replace.go:193] Ignoring instance group (master-cloud-rtp-1-c), no changes made

$ kops --name derek.k8s.local get ig nodes -o json | jq '.metadata.generation'
4

$ kops --name derek.k8s.local replace -f ~/derek.yaml
E0523 08:42:04.009326   80964 replace.go:149] Ignoring cluster (derek.k8s.local), no changes made
E0523 08:42:05.734777   80964 replace.go:193] Ignoring instance group (bastions), no changes made
E0523 08:42:07.073733   80964 replace.go:193] Ignoring instance group (master-cloud-rtp-1-a), no changes made
E0523 08:42:08.286449   80964 replace.go:193] Ignoring instance group (master-cloud-rtp-1-b), no changes made
E0523 08:42:09.652246   80964 replace.go:193] Ignoring instance group (master-cloud-rtp-1-c), no changes made
E0523 08:42:10.962620   80964 replace.go:193] Ignoring instance group (nodes), no changes made

$ kops --name derek.k8s.local get ig nodes -o json | jq '.metadata.generation'
4

@drekle
Copy link
Contributor Author

drekle commented May 23, 2019

My last commit removed any change to the functionality of the command. It will compare the spec's of an object and only increment generation if they are different. It will, as it did previously, perform the replace even if their spec's are identical.

@drekle drekle force-pushed the openstack_rolling_update branch from 7750e3c to 5d0d2a7 Compare May 23, 2019 16:32
@drekle
Copy link
Contributor Author

drekle commented May 23, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 23, 2019
Copy link
Member

@zetaab zetaab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me this looks quite ok. However, does this support kops set cluster spec.kubernetesVersion=1.14.0 for instance? At least I am using that when I am updating stuff

@drekle
Copy link
Contributor Author

drekle commented May 26, 2019

for me this looks quite ok. However, does this support kops set cluster spec.kubernetesVersion=1.14.0 for instance? At least I am using that when I am updating stuff

Yeah this is correct, I would additionally have to update the set command to increment generation. I believe this is done server side in K8s. It would be great to have as a part of the clientset, instead of doing this in each command.

@drekle drekle force-pushed the openstack_rolling_update branch from 5d0d2a7 to c723e44 Compare May 28, 2019 18:39
Ignoring replace with no spec changes

Updating replace cancellation to only not set generation, instead of not performing the update

Bazel updates

Setting generation in common clientset code

Bazel updates
@drekle drekle force-pushed the openstack_rolling_update branch from c723e44 to 2f25db8 Compare May 28, 2019 18:42
@drekle
Copy link
Contributor Author

drekle commented May 28, 2019

@zetaab I have moved the incrementation of the generation field on the cluster and instance group objects into the vfsclientset so each command will eventually cascade down into there. This now works with replace, set, edit, and likely any other commands or future commands.

@chrisz100
Copy link
Contributor

/retest

@zetaab lmk when you reviewed the changes and if it's okay from your side!

@drekle
Copy link
Contributor Author

drekle commented May 30, 2019

/test pull-kops-e2e-kubernetes-aws

Copy link
Member

@zetaab zetaab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2019
@zetaab
Copy link
Member

zetaab commented Jun 3, 2019

/test pull-kops-e2e-kubernetes-aws

@drekle
Copy link
Contributor Author

drekle commented Jun 6, 2019

@chrisz100 if this looks good would you mind approving this and perhaps reviewing these backports of the same:
#7108
#7105

@chrisz100
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, drekle

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2019
@k8s-ci-robot k8s-ci-robot merged commit 19f0da0 into kubernetes:master Jun 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/openstack Issues or PRs related to openstack provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Openstack rolling update always reports NeedsUpdate
4 participants