-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Honor OS update policy at InstanceGroup level too #10913
Honor OS update policy at InstanceGroup level too #10913
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remain uncertain whether we need to change file upup/pkg/fi/nodeup/nodetasks/package.go. At present, it pays attention to the "external" policy, but its treatment might be inverted accidentally.
@@ -133,8 +133,8 @@ type ClusterSpec struct { | |||
IsolateMasters *bool `json:"isolateMasters,omitempty"` | |||
// UpdatePolicy determines the policy for applying upgrades automatically. | |||
// Valid values: | |||
// 'external' do not apply updates automatically - they are applied manually or by an external system | |||
// missing: default policy (currently OS security upgrades that do not require a reboot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this avoidance of reboots was and is not true for Flatcar Container Linux. It only installs updates by way of rebooting. It downloads and verifies them ahead of time, but only pivots to using the new files upon reboot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked @hakman whether I should build a loader.OptionsBuilder
for this field, or to add it to an existing one, in order to set the default value to "automatic." We could consider that for the ClusterSpec
type, but should not do so for the InstanceGroupSpec
type, because the latter should default to not overriding the former, by way of making no statement about its policy.
/retest |
c306818
to
94ab4d1
Compare
I was able to rebase this cleanly, two documentation conflicts notwithstanding. PTAL. |
/retest |
@johngmyers, can you speak to the concern that I raised in this Slack message regarding |
@hakman set me straight. He's the original author of the blocks in question, and he explained that it's working as intended, but needs to be augmented to take my new InstanceGroup-level field into account. |
As with the Cluster-level "spec.updatePolicy" field, add a similar field at the InstanceGroup level, allowing overriding of the cluster-level choice in each InstanceGroup. Introduce a new value for the field ("automatic") as equivalent to the default value applied when the field is absent. Honoring this new value allows disabling automatic updates at the cluster level, but then enabling them again for particular InstanceGroups. Without such a positive affirmation, it's not possible to override a cluster-level "external" policy at the InstanceGroup level, as there's no way to specify positively that you want to recover the default value. Instead, expressing the explicit "automatic" value is clear and unambiguous.
94ab4d1
to
2fc6856
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman 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 |
…-upstream-release-1.20 Automated cherry pick of #10913: Honor OS update policy at InstanceGroup level too
As with the Cluster "spec.updatePolicy" field, add a similar field at the InstanceGroup level, allowing overriding of the cluster-level choice in each InstanceGroup.
Introduce a new value for the field ("automatic") as equivalent to the default value applied when the field is absent. Honoring this new value allows disabling automatic updates at the cluster level, but then enabling them again for particular InstanceGroups. Without such a positive affirmation, it's not possible to override a cluster-level "external" policy at the InstanceGroup level, as there's no way to specify positively that you want to recover the default value. Instead, expressing the explicit "automatic" value is clear and unambiguous.
See this thread in the "kops-users" channel of the "Kubernetes" Slack workspace for preceding discussion.
This change is