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.

Sign up for GitHub

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

Jump to bottom

Stop copying admin.conf to $HOME #936

Merged
merged 2 commits into from
Jun 26, 2020
Merged

Stop copying admin.conf to $HOME #936

merged 2 commits into from
Jun 26, 2020

Conversation

kron4eg
Copy link
Member

@kron4eg kron4eg commented Jun 25, 2020

What this PR does / why we need it:
Fixes bug on instances where $HOME is located on tmpfs and is not persisted across restarts.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #935

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Stop copying admin.conf to $HOME

@kron4eg
Stop copying admin.conf and use it directly
3d55022 
As $HOME is not always persisted (Flatcar/Coreos)

Signed-off-by: Artiom Diomin <artiom@loodse.com>
@kron4eg kron4eg requested a review from xmudrii June 25, 2020 15:47
@kubermatic-bot kubermatic-bot added dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 25, 2020
@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kron4eg

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

@kubermatic-bot kubermatic-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 25, 2020
@kron4eg
Copy link
Member Author

kron4eg commented Jun 25, 2020

/retest

@xmudrii
Copy link
Member

xmudrii commented Jun 25, 2020

With this change, we would have to run all kubectl commands with sudo, which is not the greatest practice and there were some objections against this in the past.

Why we don't just copy the kubeconfig when starting upgrade as well? That would be a simpler change that would also work with tmpfs.

@kron4eg
Copy link
Member Author

kron4eg commented Jun 25, 2020

Why we don't just copy the kubeconfig when starting upgrade as well? That would be a simpler change that would also work with tmpfs.

IDK ¯\_(ツ)_/¯, why to copy if we can not to copy?

It's easy to loose track where is what and when, meaning: copying $KUBECONFIG around will have be an "always make sure it was done" operation or make assumption that it has been done. This PR fixes one such assumption which turned to be not true.

On the other hand admin.conf location is well known and it's always there.

@kron4eg
Copy link
Member Author

kron4eg commented Jun 25, 2020

/retest

xmudrii
xmudrii reviewed Jun 26, 2020
View reviewed changes
pkg/scripts/node.go
Comment on lines +21 to +22
sudo KUBECONFIG=/etc/kubernetes/admin.conf \
kubectl drain {{ .NODE_NAME }} --ignore-daemonsets --delete-local-data
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it consistent between addons and node scripts and use either flag or the environment variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done

@xmudrii
Copy link
Member

xmudrii commented Jun 26, 2020

IDK ¯_(ツ)_/¯, why to copy if we can not to copy?

It's easy to loose track where is what and when, meaning: copying $KUBECONFIG around will have be an "always make sure it was done" operation or make assumption that it has been done. This PR fixes one such assumption which turned to be not true.

That's true, but you also have to make sure that you don't forget a flag/environment variable when running kubectl commands and that you do it in a proper way. Copying would be done once at the beginning, like we do for the clientset anyways.

However, I'm okay with whatever solution. I've left a comment on the PR if we go ahead with the proposed approach.

@kron4eg
Get rid of kubectl --kubeconfig
22b5c10 
Signed-off-by: Artiom Diomin <artiom@loodse.com>
@kubermatic-bot kubermatic-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 26, 2020
@kron4eg
Copy link
Member Author

kron4eg commented Jun 26, 2020

/retest

1 similar comment
@xmudrii
Copy link
Member

xmudrii commented Jun 26, 2020

/retest

xmudrii
xmudrii approved these changes Jun 26, 2020
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 04a3b6e3f76984add8d364b6330772bf560211aa

@kubermatic-bot kubermatic-bot merged commit d3fa740 into master Jun 26, 2020
@kubermatic-bot kubermatic-bot added this to the v1.0 milestone Jun 26, 2020
@kubermatic-bot kubermatic-bot deleted the flatcar-home-kubeconfig branch June 26, 2020 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xmudrii xmudrii xmudrii approved these changes

Assignees

@xmudrii xmudrii

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.

Upgrade process is broken on Flatcar
3 participants
@kron4eg @kubermatic-bot @xmudrii