Add s390x support #83
Conversation
|
Welcome @prabhav-thali! It looks like this is your first PR to ppc64le-cloud/k8s-ansible 🎉 |
ppc64le-cloud-bot
added
the
size/L
group_vars/all
Outdated
| - kubernetes-test-linux-{{ ansible_architecture }}.tar.gz | ||
| - kubernetes-client-linux-{{ ansible_architecture }}.tar.gz | ||
| - kubernetes-server-linux-{{ ansible_architecture }}.tar.gz |
There was a problem hiding this comment.
what will the value for this when we run this script from the x86 machine?
There was a problem hiding this comment.
The value returned is x86_64. Should we handle this for intel machine? by mapping x86_64 to amd64 in group_vars/all file and using mapped variable rather than ansible_architecture
There was a problem hiding this comment.
I think you can leave this as is and override this variable with your own stuff(s390x)
There was a problem hiding this comment.
I have reverted this change.
install-k8s.yml
Outdated
| @@ -6,8 +6,17 @@ | |||
| - runtime | |||
| - download-k8s | |||
| - install-k8s | |||
| - role: install-cni-plugins | |||
There was a problem hiding this comment.
wondering if we can move this to flannel role
There was a problem hiding this comment.
I had kept this generic because it needs to run on both master and workers for flannel.
There was a problem hiding this comment.
that is fine, but what is stopping us to move this piece of code to install-flannel role? if this is a prerequisite for flannel then it make sense to move to that role.
There was a problem hiding this comment.
I have implemented this. Let me know if it looks good
install-k8s.yml
Outdated
|
|
||
| - name: Install networking - calico | ||
| hosts: masters | ||
| roles: | ||
| - install-calico | ||
| - role: install-calico | ||
| when: ansible_architecture == "ppc64le" |
There was a problem hiding this comment.
instead of choosing arch wise, we need to find a way to choose cni type via group args because there is a possibility that you may install flannel on ppc64le as well
There was a problem hiding this comment.
Yes added cni_provider variable to handle this
roles/install-flannel/tasks/main.yml
Outdated
| @@ -0,0 +1,10 @@ | |||
| - name: Download flannel manifest | |||
| get_url: | |||
| url: "https://raw.githubusercontent.com/flannel-io/flannel/v0.26.1/Documentation/kube-flannel.yml" | |||
There was a problem hiding this comment.
try to control this version from the group vars so that you can update later in the config
There was a problem hiding this comment.
Added flannel_version variable to control this from group vars
roles/install-k8s/tasks/main.yml
Outdated
| @@ -14,6 +16,7 @@ | |||
| - name: Remove domainname from the hostname | |||
| shell: | | |||
| hostnamectl set-hostname $(hostname | cut -d "." -f1) | |||
| when: ansible_architecture == 'ppc64le' | |||
There was a problem hiding this comment.
why this is not applicable for s390x platform?
There was a problem hiding this comment.
For s390x VSIs, domainname is not appended to the hostname.
There was a problem hiding this comment.
then this code will not make any difference if it runs!
There was a problem hiding this comment.
Removed the arch-based check from here
roles/k8s-ut/tasks/main.yml
Outdated
| if [[ $(uname -m) == "s390x" ]]; then | ||
| useradd -s /bin/bash -d /home/nonroot -m nonroot | ||
| else | ||
| useradd nonroot | ||
| fi |
There was a problem hiding this comment.
can we do same instructions on both the platform?
cc @Rajalakshmi-Girish
There was a problem hiding this comment.
Tested useradd -s /bin/bash -d /home/nonroot -m nonroot on centos:stream9. It seems to work fine. Hence, removed the arch based check.
prabhav-thali
force-pushed
the
add-s390x
branch
from
123aabb to
ea80a13
Compare
There was a problem hiding this comment.
overall LGTM except one minor comment, please address that.
@Rajalakshmi-Girish ptal the code and add lgtm whenever you are comfortable.
group_vars/all
Outdated
| - kubernetes-test-linux-{{ ansible_architecture }}.tar.gz | ||
| - kubernetes-client-linux-{{ ansible_architecture }}.tar.gz | ||
| - kubernetes-server-linux-{{ ansible_architecture }}.tar.gz |
There was a problem hiding this comment.
I think you can leave this as is and override this variable with your own stuff(s390x)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mkumatag, prabhav-thali 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 |
ppc64le-cloud-bot
added
the
approved
prabhav-thali
force-pushed
the
add-s390x
branch
from
ea80a13 to
4344031
Compare
|
/lgtm |
This adds s390x support. Conditionals were added to handle the Ansible playbook execution which uses ansible_pkg_mgr and ansible_architecture variables.
Tested the same by executing unit tests, node e2e tests, and conformance tests.