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

Split guide to make easier to follow #272

Closed
wants to merge 3 commits into from

Conversation

jsturtevant
Copy link
Contributor

Signed-off-by: James Sturtevant jstur@microsoft.com

Reason for PR:

The guide had multiple CNI providers in the one doc and was hard to follow. This splits it out so it is easier to follow.

Issue Fixed:

Issue #

Requirements

  • Sqaush commits
  • Documentation
  • Tests

Notes:

@k8s-ci-robot k8s-ci-robot added 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. labels Dec 27, 2022
@jsturtevant
Copy link
Contributor Author

/assign @fabi200123

@k8s-ci-robot
Copy link
Contributor

@jsturtevant: GitHub didn't allow me to assign the following users: fabi200123.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @fabi200123

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.

@github-actions github-actions bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 27, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 27, 2022
Copy link
Contributor

@fabi200123 fabi200123 left a comment

Choose a reason for hiding this comment

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

This is much easier to follow and I believe it is better to have calico and flannel in 2 different files. Also, shall we remove the refference that this is work in progress and the reference to #239?
Also, I believe the PR name should be "Split guide..." not "Split guild..."

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabi200123, jsturtevant

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

@jsturtevant jsturtevant changed the title Split guild to make easier to follow Split guide to make easier to follow Jan 3, 2023
@jsturtevant
Copy link
Contributor Author

This is much easier to follow and I believe it is better to have calico and flannel in 2 different files. Also, shall we remove the refference that this is work in progress and the reference to #239?
Also, I believe the PR name should be "Split guide..." not "Split guild..."

done

guides/calico.md Outdated Show resolved Hide resolved
```bash
kubectl create -f https://raw.githubusercontent.com/projectcalico/calico/$CALICO_VERSION/manifests/tigera-operator.yaml
curl https://raw.githubusercontent.com/projectcalico/calico/$CALICO_VERSION/manifests/custom-resources.yaml -O
# If IP CIDR differs from 192.168.0.0/16, make sure to modify it in custom-resources.yaml.
Copy link
Contributor

@marosset marosset Jan 4, 2023

Choose a reason for hiding this comment

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

This is the cluster CIDR right (not pod cidr)?
We should specify here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the pod CIDR. I didn't modify this aspect of the guide. @fabi200123 do you know off hand?

Now you can add Windows-compatible versions of calico and kube-proxy. In order to ensure that you get a compatible version of kube-proxy, you'll need to substitute the tag of the image. The following example shows usage for Kubernetes v1.25.3, but you should adjust the version for your own deployment.

```bash
curl -L https://raw.githubusercontent.com/kubernetes-sigs/sig-windows-tools/master/hostprocess/calico/kube-proxy/kube-proxy.yml | sed 's/KUBE_PROXY_VERSION/v1.25.3/g' | kubectl apply -f -
Copy link
Contributor

Choose a reason for hiding this comment

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

nit should we export KUBE_PROXY_VERSION and CALICO_VERSION env vars at the beginning so it is more obvious which values need to be set by the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's follow up with this, I this was a copy/paste of what is there currently to make it easier to follow/maintain

guides/flannel.md Outdated Show resolved Hide resolved
Now you can add Windows-compatible versions of Flannel and kube-proxy. In order to ensure that you get a compatible version of kube-proxy, you'll need to substitute the tag of the image. The following example shows usage for Kubernetes v1.24.3, but you should adjust the version for your own deployment.

```bash
curl -L https://raw.githubusercontent.com/kubernetes-sigs/sig-windows-tools/master/hostprocess/flannel/kube-proxy/kube-proxy.yml | sed 's/KUBE_PROXY_VERSION/v1.25.3/g' | kubectl apply -f -
Copy link
Contributor

Choose a reason for hiding this comment

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

## Before you begin

Your Kubernetes server must be at or later than version 1.22. To check the version, enter `kubectl version`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will be nice to add a link to setting up the Linux master node here too; perhaps to the official upstream docs? This way, this page can be a one-stop shop for fresh set up. I see that's already mentioned in bullet 2 below, perhaps that should be the first bullet?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just went through setting up the master node on a Hyper-V Ubuntu 22.04 VM and it was blood and tears; you don't want to go over it again -- but I want to challenge myself to do a quickstart guide that just highlights the pitfalls and common issues. Later, I would like to do an unattended shell script for setting up the master node (for dev purposes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just went through setting up the master node on a Hyper-V Ubuntu 22.04 VM and it was blood and tears;

Yes setting up a cluster from scratch isn't a trivial game. There are tons of vendors and providers who attempt to solve this problem.

## Before you begin

Your Kubernetes server must be at or later than version 1.22. To check the version, enter `kubectl version`.
Your Kubernetes cluster must be at or later than version 1.23. To check the version, enter `kubectl version`.

- Obtain a [Windows Server 2019 license](https://www.microsoft.com/en-us/cloud-platform/windows-server-pricing) (or higher) in order to configure the Windows node that hosts Windows containers. If you are using VXLAN/Overlay networking you must have also have [KB4489899](https://support.microsoft.com/help/4489899) installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the license needed? I'm setting up mine a 2022 Evaluation Copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can use an evaluation copy for testing purposes

### Joining a Windows worker node

> **Note:** All code snippets in Windows sections are to be run in a PowerShell environment with elevated permissions (Administrator) on the Windows worker node.

1. Install ContainerD, wins, kubelet, and kubeadm.
1. Install ContainerD, kubelet, and kubeadm.

```PowerShell
# Install ContainerD
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: will be nice for Install -Containerd.ps1 to require the version other than having 6.1.8 as default as it's fast becoming outdated. We are now at 6.1.15. Perhaps to recommend that they have same version as they one they set up on the master node? And for dev purposes, latest is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self, I'll need to investigate this further: within the VM, curl.exe kept on failing consistently (just drops connection). I had to switch to Invoke-WebRequest -Uri $source -OutFile $destination which worked okay. I'll need to investigate a little more on what could have been happening.

### Joining a Windows worker node

> **Note:** All code snippets in Windows sections are to be run in a PowerShell environment with elevated permissions (Administrator) on the Windows worker node.

1. Install ContainerD, wins, kubelet, and kubeadm.
1. Install ContainerD, kubelet, and kubeadm.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add Git installation step before .\PrepareNode.ps1 step since it's depended on it.

C:\Users\Administrator\k8s-setup\PrepareNode.ps1 : The term 'git' is not recognized as the name of a cmdlet, function,
script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is
correct and try again.
At line:1 char:1
+ .\PrepareNode.ps1 -KubernetesVersion v1.26.0
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make .\PrepareNode.ps1 script idempotent; fails after running the second time when an initially created file is re-created:

----                 -------------         ------ ----
d-----         1/22/2023  10:28 PM                pki
New-Item : NewItemIOError
At C:\Users\Administrator\k8s-setup\PrepareNode.ps1:61 char:1
+ New-Item -path C:\var\lib\kubelet\etc\kubernetes\pki -type SymbolicLi ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ResourceExists: (C:\var\lib\kubelet\etc\kubernetes\pki:String) [New-Item], IOException
    + FullyQualifiedErrorId : NewItemIOError,Microsoft.PowerShell.Commands.NewItemCommand

Actually it's only these 2 other places:

Registering kubelet service
Error creating service!
CreateService(): The specified service already exists.

Set parameter "DependOnService" for service "kubelet".
New-NetFirewallRule : Cannot create a file when that file already exists.
At C:\Users\Administrator\k8s-setup\PrepareNode.ps1:106 char:1
+ New-NetFirewallRule -Name kubelet -DisplayName 'kubelet' -Enabled Tru ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ResourceExists: (MSFT_NetFirewallRule:root/standardcimv2/MSFT_NetFirewallRule) [New-NetF
   irewallRule], CimException
    + FullyQualifiedErrorId : Windows System Error 183,New-NetFirewallRule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: add Git installation step before .\PrepareNode.ps1 step since it's depended on it.

#280 should resolve this

nit: make .\PrepareNode.ps1 script idempotent; fails after running the second time when an initially created file is re-created:

let's do a follow up for this. This PR is just a copy/paste so it is easier to maintain

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2023
@riverar
Copy link

riverar commented Apr 13, 2023

Nudging this PR as it's getting stale and blocking important documentation updates.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 14, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 14, 2023
@jsturtevant
Copy link
Contributor Author

/assign @marosset
Since this is a straight copy paste, I would like to get it in so we can then make some of the valid suggestion made above

@Mik4sa
Copy link
Contributor

Mik4sa commented May 15, 2023

What's the state here?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@jsturtevant
Copy link
Contributor Author

will need to update since some changes went in. If you want we can merge your other PR's and you can submit a PR like this and I will be able to merge it since it is a straight move to split the files out.

@Mik4sa
Copy link
Contributor

Mik4sa commented May 16, 2023

@jsturtevant I just did so :).

@jsturtevant
Copy link
Contributor Author

/close

@k8s-ci-robot
Copy link
Contributor

@jsturtevant: Closed this PR.

In response to this:

/close

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.

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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants