Skip to content
This repository has been archived by the owner on Mar 15, 2021. It is now read-only.

Fix sk8 to pass conformance #15

Merged
merged 3 commits into from Jun 6, 2019
Merged

Conversation

yastij
Copy link
Contributor

@yastij yastij commented Jun 6, 2019

This change updates sk8 to pass conformance, change is minimal and commits are reviewable separately

cc @akutz

Signed-off-by: Yassine TIJANI <ytijani@vmware.com>
Signed-off-by: Yassine TIJANI <ytijani@vmware.com>
Signed-off-by: Yassine TIJANI <ytijani@vmware.com>
Copy link
Contributor

@akutz akutz left a comment

Choose a reason for hiding this comment

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

Hi @yastij,

This is great! Thank you! I just had a few nits to pick, but other than that, looks good. Great job!

cc @clintkitson

@@ -590,7 +590,7 @@ turn_up() {
if [ "${EXTERNAL}" = "true" ]; then
printf "waiting for cluster to finish coming online... "
i=0 && while true; do
[ "${i}" -ge 100 ] && { error "timed out waiting for cluster" 1; return; }
[ "${i}" -ge 110 ] && { error "timed out waiting for cluster" 1; return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yastij,

Hmmm, a loop iteration count of 100 equals 5 minutes. Should we just double it to 200 for 10 minutes? Any reason not to?

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 we'll be fine with this. I can revisit if not enough, but nothing against.

@@ -2574,8 +2574,7 @@ EOF
fi

cat <<EOF >/etc/default/kubelet
KUBELET_OPTS="--allow-privileged \\
--client-ca-file='${TLS_CA_CRT}'${EXT_CLOUD_PROVIDER_OPTS} \\
KUBELET_OPTS="--client-ca-file='${TLS_CA_CRT}'${EXT_CLOUD_PROVIDER_OPTS} \\
Copy link
Contributor

@akutz akutz Jun 6, 2019

Choose a reason for hiding this comment

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

Hi @yastij,

Any negative effect(s) on versions of Kubernetes built before this commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be fine as it already defaults to true to supported version see kubernetes/kubernetes#63442

@akutz
Copy link
Contributor

akutz commented Jun 6, 2019

Hi @yastij,

One last note -- there are five locations in sk8.sh where -gt 100 is used. Should those be updated as well do you think? I need to think about it -- which of those depend on the external LB. I actually don't think any do because of the hairpin protection, but I just wanted to mention it.

@yastij
Copy link
Contributor Author

yastij commented Jun 6, 2019

@akutz - I think that the others are fine, it's mostly LBs that take time

@akutz akutz merged commit f55b86e into vmware-archive:master Jun 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants