-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Improve troubleshooting guide for issues switching from Docker Engine #34764
Conversation
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Few grammatical nits + incorporated suggestions from @Sea-n
Hi @divya-mohan0209 @Sea-n , Thank you for suggesting the changes. The following changes have been added now. |
@@ -14,7 +14,23 @@ problem you are experiencing. See | |||
the [application troubleshooting guide](/docs/tasks/debug/debug-application/) for tips on application debugging. | |||
You may also visit the [troubleshooting overview document](/docs/tasks/debug/) for more information. | |||
|
|||
Before upgrading to Kubernetes 1.24 , please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-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.
nit: You can remove the word, 'please'.
Before you upgrade to Kubernetes 1.24, review the Urgent 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.
You should incorporate this feedback @nitishkumar06
Also, we ought to remove this line on the dev-1.25 branch, once that branch has merged in this update.
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 like the intent here, but the advice is misleading enough that we should not merge this as-is.
@nitishkumar06 do you understand how to fix those concerns?
kubelet --help | grep network-plugin | ||
``` | ||
|
||
*Note: If you get the following error after upgrading to Kubernetes 1.24, go through the [Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/) guide to get a better understanding of how to resolve this error.* |
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: If you get the following error after upgrading to Kubernetes 1.24, go through the [Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/) guide to get a better understanding of how to resolve this error.* | |
If you get the following error after upgrading to Kubernetes 1.24, and you were previously using Docker Engine as your container runtime, go through the [Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/) guide. |
|
||
*Note: If you get the following error after upgrading to Kubernetes 1.24, go through the [Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/) guide to get a better understanding of how to resolve this error.* | ||
|
||
```shell |
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 not a shell script. Omit shell
here.
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 issue is still not addressed.
To start your cluster, run the following command: | ||
|
||
```shell | ||
kubelet --help | grep network-plugin | ||
``` |
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 not a command that starts your cluster
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 @sftim , I'll push the new changes now.
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.
@sftim The new changes have been pushed. I've deleted the following command which is wrong and believe it can go without mentioning on "How to start a cluster" since it's error resolving page and not "guide page"
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 incorrect
You do not start your cluster with kubelet --help | grep network-plugin
/hold
Holding until this is corrected
WDYT of this change now? @sftim |
/retitle Improve troubleshooting guide for issues switching from Docker Engine |
To me, #34764 (comment) is only barely a nit. It really would be better to remove “please”; that word isn't right for a troubleshooting guide. However, this change is much more right than wrong. |
LGTM label has been added. Git tree hash: 0f4c2bfcef3c6289f1170bc6dca3e506eb6ea149
|
@sftim Ahan sorry! I forgot to do that. I'll do it now. |
New changes are detected. LGTM label has been removed. |
@sftim The requested changes are applied now. I think this PR is ready to be merged. WDYT? |
Hey @nitishkumar06, It's been a while since the last update, should we close this PR? If you want to change the target branch to |
Before upgrading to Kubernetes 1.24, please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-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.
The current main
branch is for 1.25
I'm also ok with this line not being in the top of this page as there is no other content/context around troubleshooting upgrades.
Maybe a warning like this (sans version) can go in the Upgrade a Cluster. A similar sentence is already in the Upgrading kubeadm clusters page
Before upgrading to Kubernetes 1.24, please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-upgrade-notes). | |
Before upgrading to Kubernetes 1.24, please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-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.
@reylejano Thank you for the suggestions! I'd be committing the changes soon.
@nitishkumar06 |
The target branch for the 1.24 docs is the |
@reylejano Please checkout the file changes in the latest commit. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@sftim @reylejano @Sea-n Kindly take a look at the changes now. |
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.
Left some suggestions. WDYT?
@@ -27,6 +27,8 @@ the documentation for the version of Kubernetes that you plan to upgrade to. | |||
|
|||
## Upgrade approaches | |||
|
|||
please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-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.
I've a couple of suggestions here.
- I'd recommend not using the word please here. 2. We're linking to the 1.24 upgrade notes for a particular reason right? We should say why we're asking our audience to go through the guide as a form of context setting.
|
||
## {{< note >}} | ||
*Note: Due to change in CNI, the `kubelet` binary might not recognize `--network-plugin` flag even after passing to `kubelet` which leads to the following error: |
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.
IMO, this could be phrased better. WDYT?
*Note: Due to change in CNI, the `kubelet` binary might not recognize `--network-plugin` flag even after passing to `kubelet` which leads to the following error: | |
*Note: Starting 1.24, since management of the CNI is no longer in scope for kubelet, the `--network-plugin` command line parameter has been removed from the binary and will throw the following error, if used: |
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.
As per @reylejano suggested here, I've omitted this line.
Hello @nitishkumar06 . |
@kbhawkey: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
How does it make sense to open a new PR? This PR already had the suggested changes pushed. We were looking forward to getting a review from the maintainers before proceeding further rather than closing this one. |
Hello @nitishkumar06 . You can always reopen a pull request. Do you want to mark this PR as a draft or are the changes ready to merge? |
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'm not sure why we have removed the heading 2 at line 29.
I'm not sure why we add a heading 3 at line 317.
Line 319-325 is a note, that is fine. But line 319 and 325 are marked heading 2?
Why line 320 starts with a '*'?
Can we wrap the lines inside the note?
This workspace kind of looks messed up - Hence created a new PR: #39625 |
Fix: #34756
Note
The approach that I followed was to add a small paragraph at the beginning of the page (https://kubernetes.io/docs/tasks/debug/debug-cluster/) Cluster not starting which will guide the users to navigate to the following page (https://kubernetes.io/docs/tasks/administer-cluster/migrating-from-dockershim/) to understand more about the error:
Error: failed to parse kubelet flag: unknown flag: --network-plugin
Also, Before upgrading to Kubernetes 1.24, it is essential to go through Urgent Upgrade Notes . This could also be seen from @sftim comment on the issue that this PR is fixing.
Please feel free to suggest changes if necessary.