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

Fix libvirt-coreos cluster #11379

Merged
merged 1 commit into from Jul 24, 2015
Merged

Conversation

lhuard1A
Copy link

KUBERNETES=libvirt-coreos cluster/kube-up.sh produced the following error:

cluster/../cluster/libvirt-coreos/../../cluster/common.sh: line 83: user_args[@]: unbound variable

This was coming from the fact that, as a libvirt-coreos cluster runs locally on local VMs,
there is no authentication mechanism. This led to have user_args of common.sh unset.

In the case of libvirt-coreos, it is in fact expected to have no authentication token.

KUBERNETES=libvirt-coreos cluster/kube-up.sh produced the following error:

cluster/../cluster/libvirt-coreos/../../cluster/common.sh: line 83: user_args[@]: unbound variable

This was coming from the fact that, as a libvirt-coreos cluster runs locally on local VMs,
there is no authentication mechanism. This led to have user_args of common.sh unset.

In the case of libvirt-coreos, it is in fact expected to have no authentication token.
@lhuard1A
Copy link
Author

Hi @sdminonne ,
Could you check if this PR fixes the issue you tried to fix in #10228 ?

@k8s-bot
Copy link

k8s-bot commented Jul 16, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@sdminonne
Copy link
Contributor

@lhuard1A sure. Let you know ASAP.

@sdminonne
Copy link
Contributor

@lhuard1A LGTM (i.e. it works on my side) 👍

@zmerlynn may you have a look?

@brenix
Copy link

brenix commented Jul 21, 2015

Works here. Thanks!

@@ -80,7 +80,9 @@ function create-kubeconfig() {
fi

"${kubectl}" config set-cluster "${CONTEXT}" "${cluster_args[@]}"
"${kubectl}" config set-credentials "${CONTEXT}" "${user_args[@]}"
if [ -n "${user_args[@]:-}" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The rest of the file does 2 space indentation as well.

Copy link
Member

Choose a reason for hiding this comment

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

in my limited testing, I think using [ instead of [[ is what broke this for GCE, though I can't entirely explain why.

Copy link
Member

Choose a reason for hiding this comment

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

These actually evaluate differently. The [[ -n does the right thing with array expansion, whereas [ seems to, well, deal poorly. I love bash:

zml@zml:~$ args=("--foo" "--bar" "--blatz")
zml@zml:~$ if [[ -n "${args[@]:-}" ]]; then echo "WUMPUS"; fi
WUMPUS
zml@zml:~$ if [ -n "${args[@]:-}" ]; then echo "WUMPUS"; fi
-bash: [: too many arguments

Copy link
Member

Choose a reason for hiding this comment

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

that's what I discovered in my testing, too, but I didn't see that error in the build log on Jenkins, so I was/am confused.

Copy link
Member

Choose a reason for hiding this comment

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

wait no, I'm just bad at searching the log:

/jenkins-master-data/jobs/kubernetes-e2e-gce-parallel/workspace/kubernetes/hack/e2e-internal/../../cluster/../cluster/gce/../../cluster/common.sh: line 83: [: too many arguments

@mikedanese
Copy link
Member

LGTM

@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2015
@mikedanese mikedanese self-assigned this Jul 23, 2015
@vishh vishh closed this Jul 24, 2015
@vishh vishh reopened this Jul 24, 2015
@mikedanese
Copy link
Member

All green, and isolated change

mikedanese added a commit that referenced this pull request Jul 24, 2015
@mikedanese mikedanese merged commit 788012a into kubernetes:master Jul 24, 2015
@ixdy
Copy link
Member

ixdy commented Jul 24, 2015

Pretty sure this change broke Jenkins for GCE:

... calling validate-cluster.sh
error: couldn't read version from server: the server has asked for the client to provide credentials

Waiting for 6 ready nodes. 0 ready nodes, 0 registered. Retrying.
error: couldn't read version from server: the server has asked for the client to provide credentials

Waiting for 6 ready nodes. 0 ready nodes, 0 registered. Retrying.
error: couldn't read version from server: the server has asked for the client to provide credentials
...

This change also didn't run through the per-PR Jenkins before being merged, making it even more suspect.

@mikedanese
Copy link
Member

Oh man, I missed that. I apologize. Shame on me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants