-
Notifications
You must be signed in to change notification settings - Fork 117
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
Freeze the version of kubelet, kubectl and kubeadm #273
Conversation
lib/common.sh
Outdated
@@ -111,6 +111,7 @@ export DEFAULT_HOSTS_MEMORY=${DEFAULT_HOSTS_MEMORY:-8192} | |||
export CLUSTER_NAME=${CLUSTER_NAME:-"test1"} | |||
export CLUSTER_APIENDPOINT_IP=${CLUSTER_APIENDPOINT_IP:-"192.168.111.249"} | |||
export KUBERNETES_VERSION=${KUBERNETES_VERSION:-"v1.18.0"} | |||
export K3_BINARIES_VERSION=${K3_BINARIES_VERSION:-"1.18.0"} |
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.
why not to use KUBERNETES_VERSION variable ?
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.
They are not in the same format. (v)
:-"v1.18.0"
:-"1.18.0"
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 see, but then you should change also K3_BINARIES_VERSION to be default('1.18.0', true) }}"
in vm-setup/roles/v1aX_integration_test/vars/main.yml , no?
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 I have done so.
It is included in this commit
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 could make KUBERNETES_VERSION to be 1.18.0 and then wherever used add the v in front, that would be better than creating this duplicated variable
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.
ok. I will look into that.
d931ccf
to
6482d81
Compare
/test-integration |
1 similar comment
/test-integration |
3730c11
to
d3b9281
Compare
/test-integration |
why K3 btw ? |
now you are missing the setting of the variable in the lib/common.sh. Kubernetes version is set there, we should have both at the same place. Also, if you add something into lib/common.sh, it should be considered whether that should be added in config_example.sh or not and on the try-it.md of the website |
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.
why K3 btw ?
It stands for the three binaries. kubeadm, kubectl and kubelet.
Also tried 3K, but it did not accept it.
And, I thought it would be too long to write KUBEADM_KUBELET_KUBECTL_VERSION
lib/common.sh
Outdated
@@ -111,6 +111,7 @@ export DEFAULT_HOSTS_MEMORY=${DEFAULT_HOSTS_MEMORY:-8192} | |||
export CLUSTER_NAME=${CLUSTER_NAME:-"test1"} | |||
export CLUSTER_APIENDPOINT_IP=${CLUSTER_APIENDPOINT_IP:-"192.168.111.249"} | |||
export KUBERNETES_VERSION=${KUBERNETES_VERSION:-"v1.18.0"} | |||
export K3_BINARIES_VERSION=${K3_BINARIES_VERSION:-"1.18.0"} |
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 I have done so.
It is included in this commit
lib/common.sh
Outdated
@@ -111,6 +111,7 @@ export DEFAULT_HOSTS_MEMORY=${DEFAULT_HOSTS_MEMORY:-8192} | |||
export CLUSTER_NAME=${CLUSTER_NAME:-"test1"} | |||
export CLUSTER_APIENDPOINT_IP=${CLUSTER_APIENDPOINT_IP:-"192.168.111.249"} | |||
export KUBERNETES_VERSION=${KUBERNETES_VERSION:-"v1.18.0"} | |||
export K3_BINARIES_VERSION=${K3_BINARIES_VERSION:-"1.18.0"} |
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.
ok. I will look into that.
d3b9281
to
f66a90a
Compare
@maelk |
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.
lgtm, just a nit, it could still be KUBERNETES_BINARIES_VERSION, and could default to KUBERNETES_BINARIES_VERSION="${KUBERNETES_BINARIES_VERSION:-${KUBERNETES_VERSION#'v'}}"
that will just remove the v from the K8S version
I think it is obvious that kubernetes binaries cover kubeadm kubectl and kubelet |
I agree. We can remove it. It is needed for upgrading automated tests. Even in that it is only test case. |
f66a90a
to
e7c91b7
Compare
/lgtm |
/tesct-centos-integration |
e7c91b7
to
12a8cf3
Compare
Changed accordingly. |
c58548d
to
aa8da1f
Compare
/test-integration |
/test-centos-integration |
/test-integration |
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.
ok for me, but I think we should rather go with K8S 1.18.0 now that the tests are passing
aa8da1f
to
cfacbf5
Compare
Thanks. |
/test-integration |
/test-centos-integration |
please do wait for test result with 1.18.0 (current code) before giving lgtm or approval |
/test-centos-integration |
/test-centos-integration |
/test-integration |
The test with v1.18.0 is failing due to missing files, unlike previous releases. I suggest we hold this until the issue is resolved. |
Fixes metal3-io#271 Signed-off-by: Anwar Hassen <anwar.hassen@est.tech>
cfacbf5
to
8135727
Compare
/test-centos-integration |
/test-integration |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maelk, Xenwar 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 |
As we have it now,
KUBERNETES_VERSION
is fixed in time. However, the binaries (kubelet, kubeadm and kubectl), are always the latest and this causes a mismatch between the two.The purpose of this PR is then making both freeze in time and prevent auto update/upgrade of both. This allows developers to keep using specific version, if they wish to do so.
In future works, we intend to use the metal3-dev-env for testing upgrade workflow, where the kuberenetes and binaries upgrade is done independently. To that end, we have added a new variable.
KUBERNETES_BINARIES_VERSION
which defaults to the value ofKUBERNETES_VERSION
This PR makes the versions both of
KUBERNETES_VERSION
andKUBERNETES_BINARIES_VERSION
set to1.17.0
only for testing purpose.Fixes #271