-
Notifications
You must be signed in to change notification settings - Fork 14
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
Charm Upgrade documentation #881
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.
Let me know when you finish writing this and I will review again but for now here are some initial thoughts
[juju-docs]: https://juju.is/docs/juju/upgrade-models | ||
[release-notes]: ../reference/releases.md | ||
[upgrade-notes]: ../reference/upgrade-notes.md | ||
[upstream-notes]: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.31.md#deprecation |
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.
Linking to a specific changelog version will make this link out of date with each release. Maybe we should like to one level higher? Or would this be updated on each release?
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.
it should be updated ever release. is there a variable for which release this is?
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.
There is a {{version}} variable we have set up like {{product}} but I am not sure how it will integrate into a url
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.
@nhennigan is there anything i can do to test it?
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.
So I tried to use {{version}} in the url but it wasn't working. There does seem to be a work around but it will take a new configuration file and I think that is out of the scope of this PR. I will add it to the backlog to see if it can be automated in the future
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.
@nhennigan would you like me to update the link to point to 1.32 changelog?
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.
Yes I think its safe enough to update to 1.32 now
It is recommended that you keep your Kubernetes deployment | ||
updated to the latest available stable version. You should | ||
also update the other applications which make up Kubernetes. |
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 would remove the second sentence and rephrase, or specify what other applications make up Kubernetes.
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.
thanks @eaudetcobello i'm opened to rewording this. I guess i could list possible other charm integrations here ceph-csi
, cinder-csi
, openstack-integrator
but there's like 60 charms and i'd rather not list them all.
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.
Then you could say "the applications you've deployed in your Kubernetes cluster"?
It is also important to understand that Kubernetes will only | ||
upgrade and if necessary migrate, components relating specifically | ||
to elements of Kubernetes installed and configured as part of Kubernetes. | ||
This may not include any customized configuration of Kubernetes, | ||
or user generated objects (e.g. storage classes) or deployments which | ||
rely on deprecated APIs. |
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 think this is confusing for a CK8s user. How does this relate to Canonical K8s specifically and the components we deploy like the built-in features?
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.
maybe so. I'm forking the instructions here
https://ubuntu.com/kubernetes/docs/upgrading
Whats good for the goose i'd hoped would be good for the gander
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.
other integrations like:
- ceph-csi
- cinder-csi
- openstack-cloud-controller-manager
none of these get upgraded when you update the k8s and k8s-worker. This statement is here for that reason
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.
Similarly -- if someone has "kubelet-extra-args" set on their application. And those args for kubelet are deprecated -- we aren't magically migrating them.
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.
Great work, thanks @addyess! The main thing I would improve here is the distinction between Kubernetes itself and Canonical Kubernetes. For example, the following paragraph:
It is also important to understand that Kubernetes will only
upgrade and if necessary migrate, components relating specifically
to elements of Kubernetes installed and configured as part of Kubernetes.
This may not include any customized configuration of Kubernetes,
or user generated objects (e.g. storage classes) or deployments which
rely on deprecated APIs.
7e03866
to
2605c98
Compare
@nhennigan
Why are there two dictionaries? |
.custom_wordlist was what we used for the automatic_doc_checks runner (complete list of checks for docs). It is basically duplicated in docs/canonicalk8s/styles/config/vocabularies/Canonical/accept.txt for the spellcheck runner. I will consolidate again into one when the automatic_doc_checks are working properly. I have plans in the coming weeks to fix the other checks that are failing like link checker, style etc. but I wanted a spellcheck on the repo in the mean time |
3e39e26
to
71693fd
Compare
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.
Great work! Thanks for taking the lead on the documentation. I do have a few suggestions:
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.
Fantastic work Adam! I've got some word nits for you please apply these to both docs.
and security patches for smooth operation of your cluster. | ||
|
||
New minor versions of Kubernetes are set to release three | ||
times per year. You can check the latest release version |
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.
times per year. You can check the latest release version | |
times per year. Check the latest release version |
(Avoid using you)
whether to a patch or minor version. | ||
``` | ||
|
||
You can see which version of each application is currently deployed by running: |
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.
You can see which version of each application is currently deployed by running: | |
Determine which version of each application is currently deployed by running: |
unforeseen difficulties. It is highly recommended that you make | ||
a backup of any important data, including any running workloads. |
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.
unforeseen difficulties. It is highly recommended that you make | |
a backup of any important data, including any running workloads. | |
unforeseen difficulties. It is highly recommended to make | |
a backup of any important data, including any running workloads. |
[docs on backups][backup-restore]. | ||
|
||
|
||
You should also make sure: |
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.
You should also make sure: | |
Please verify that: |
c0d5efa
to
4b9927a
Compare
Co-authored-by: Mateo Florido <[email protected]>
4b9927a
to
735cfe9
Compare
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 know there seems to be so many rounds of review on this but they are almost there! Once these are addressed and Louise's comments are closed I think we are good to merge 😺
[juju-docs]: https://juju.is/docs/juju/upgrade-models | ||
[release-notes]: ../reference/releases.md | ||
[upgrade-notes]: ../reference/upgrade-notes.md | ||
[upstream-notes]: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.31.md#deprecation |
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.
Yes I think its safe enough to update to 1.32 now
* Your cluster is running normally | ||
* Your Juju client and controller/models are running the same, | ||
stable version of Juju (see the [Juju docs][juju-docs]) | ||
* You read the [Upgrade notes][upgrade-notes] to see if any | ||
caveats apply to the versions you are upgrading to/from | ||
* You read the [Upstream release notes][upstream-notes] for details | ||
of Kubernetes deprecation notices and API changes that may impact | ||
your workloads |
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.
The difference is small but I think "you should" lends itself more to tutorials. How-to guides are more direct so I would implement Louise's changes.
stable release in this channel and you should continue with the | ||
[Pre Upgrade Check](#the-pre-upgrade-check). | ||
|
||
Otherwise, continue with the [Upgrade Patch](upgrade-patch) instructions. |
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 am a bit confused to the logic here. Am I right in saying:
If there can-upgrade-to is null, we are on the latest stable release (up to date in our patches ) in our channel and we can go to to the next channel with the upgrade.
If the can-upgrade-to is not null, we want them to upgrade the patch first.
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.
That's correct. I wonder how to make this more clear
[cluster-validation]: ./validate | ||
[juju-docs]: https://juju.is/docs/juju/upgrade-models | ||
[release-notes]: ../reference/releases | ||
[upgrade-notes]: ../reference/upgrade-notes |
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.
Same as before
``` | ||
|
||
The `refresh` command instructs the juju controller to follow a new | ||
charm channel related to the Kubernetes release and use the new charm |
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.
This is quite a long sentence. Can we split into two? Suggestion:
The refresh
command instructs the juju controller to follow a new
charm channel related to the Kubernetes release. It also instructs the juju controller to use the new charm revision of the application's channel to upgrade each unit.
@nhennigan i think i've incorporated your suggestions. Feel free to push any other suggestions to the branch or we can carry over this task |
seems all comments are resolved and @nhennigan approved - merging this. |
Overview
Details