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

Install packages updates during bootstrap #8635

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

hakman
Copy link
Member

@hakman hakman commented Feb 27, 2020

During bootstrap packages that are already installed are skipped. They are later installed by the automatic updater.

The PR checks if automatic updates are enabled and tries to install the updated version.
The package managed decides if there is an update to be installed or not, so the only thing that has to be done is try to install it again. The newer the base image, the less updates will be installed.

This is a followup of #8020 and after discussing about the issue during office hours.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 27, 2020
@hakman
Copy link
Member Author

hakman commented Feb 27, 2020

/cc @geojaz

Copy link
Member

@geojaz geojaz left a comment

Choose a reason for hiding this comment

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

How is tags.TagUpdatePolicyAuto getting set? Or does this change the default behavior? (We should probably avoid that)

I think either this PR (or I) may be missing something. But in general, I'd like to see a bit of documentation about how to enable. It may also be nice to log that this mode has been enabled so users have another way to figure out what's up.
Thanks!

@hakman
Copy link
Member Author

hakman commented Feb 27, 2020

Kops has a feature that does automatic updates on each node. This is enabled by default and can be disabled by setting Cluster.Spec.UpdatePolicy to external:
There is some documentation for the option here:
https://github.com/kubernetes/kops/blob/master/docs/getting_started/arguments.md#updatepolicy

The TagUpdatePolicyAuto is an alias for _automatic_upgrades, which is added only when no UpdatePolicy is set:

switch fi.StringValue(cluster.Spec.UpdatePolicy) {
case "": // default
tags.Insert("_automatic_upgrades")

Instead of waiting for the updater to run (daily), I am trying to update the packages required for Kubernetes during bootstrap.

We discussed this during office hours about a month ago and decided that should be ok to update packages during bootstrap, as long as automatic updates are not enabled.

@geojaz
Copy link
Member

geojaz commented Feb 27, 2020

ahh! ok, thanks for connecting the dots for me- It seems that I keep asking for clarification on things that were discussed during office hours... I'll try to do better at keeping up with these 🙄

I would still love a quick comment (in tags.go) that reminds people who are scanning the code of the implications of _automatic_upgrades. Something simple like: Nodes with this tag attempt to update packages during bootstrapping instead of daily . See: ... for other options. is that overkill?
I'll take another look in the US daytime :)

@hakman
Copy link
Member Author

hakman commented Feb 27, 2020

I am sure that you have better things to do than remember all the tiny nits from 6 weeks ago. :)
Adding comments sounds very reasonable. Will add some and continue this later.
Thanks!

@hakman
Copy link
Member Author

hakman commented Feb 28, 2020

@geojaz I added the comment and an extra note in docs. I think it's ok for another look. Thanks!

Comment on lines 28 to 32

// Nodes with the "_automatic_upgrade" tag will update packages for security,
// but also during bootstrapping. To disable automatic updates set
// `Cluster.Spec.UpdatePolicy = external`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Nodes with the "_automatic_upgrade" tag will update packages for security,
// but also during bootstrapping. To disable automatic updates set
// `Cluster.Spec.UpdatePolicy = external`
// Nodes with the "_automatic_upgrade" tag automatically update packages
// during bootstrapping and for security updates (unless this update would require
// a node reboot. To disable automatic node package updates, set:
// `Cluster.Spec.UpdatePolicy = external`

Copy link
Member

@geojaz geojaz left a comment

Choose a reason for hiding this comment

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

This lgtm, but had a suggestion on wording of the comment that may make it more approachable for native english speakers. Hopefully for non-native as well. :) Feel free to suggest a further change, but when you're ready, i'll 👍 this. thanks!

@hakman
Copy link
Member Author

hakman commented Feb 28, 2020

Can't imagine how I could have written such nonsense. Even I don't understand it anymore. :))

Copy link
Member

@geojaz geojaz left a comment

Choose a reason for hiding this comment

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

/lgtm
thanks @hakman !

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2020
@hakman
Copy link
Member Author

hakman commented Feb 28, 2020

Thanks @geojaz!
Btw, can you also add the approved label?

@geojaz
Copy link
Member

geojaz commented Feb 28, 2020

Teamwork! I'm just so glad that I don't have to write docs/comments in (your native language- which I don't want to guess). Ahh... I understand why you need an approve as well as a lgtm, but I don't. It's because it automatically adds approved to mine.... not fair.
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geojaz, 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 /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 Feb 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3b54cfc into kubernetes:master Feb 28, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 28, 2020
@hakman hakman deleted the install-package-updates branch February 29, 2020 07:12
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. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants