Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Bash improvements #217

Merged
merged 5 commits into from
Jan 12, 2017
Merged

Conversation

redbaron
Copy link
Contributor

@redbaron redbaron commented Jan 8, 2017

Set of changes to for cleaner and more correct bash scripts.

Mainly changes are about adding -e to make startup scripts exit on errors encountered, which would prevent things like #215 (comment) and also some cosmetics where ; \ noise is removed where possible

@codecov-io
Copy link

codecov-io commented Jan 8, 2017

Current coverage is 68.89% (diff: 100%)

Merging #217 into master will not change coverage

@@             master       #217   diff @@
==========================================
  Files             4          4          
  Lines          1135       1135          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            782        782          
  Misses          263        263          
  Partials         90         90          

Powered by Codecov. Last update 2aa13f8...5a63386

@redbaron
Copy link
Contributor Author

redbaron commented Jan 8, 2017

Few things which I didn't change, but looked strange to me:

  • seemingly unnecessary sudo rkt calls from script which already run as root as they are called from systemd service files.
  • etcd decrypts assets differently from how it is done for controller and workers
  • shouldn't it be an error if *.pem.enc files are not found? current behavior which I kept is to ignore their absence
  • install-kube-system script uploads multiple manifests to k8s all in one script,and in case of an error in one of them systemd will restart install-kube-system.service, which executes script again and triggers re-upload of previously uploaded manifests even those which were successful, how k8s react to it? Is it safe? will it create one more copy of a resource or it will have no effect?

@mumoshu mumoshu added this to the v0.9.3-rc.4 milestone Jan 10, 2017
@@ -466,8 +465,6 @@ write_files:
done;
echo done.'

sudo rkt rm --uuid-file=/var/run/coreos/decrypt-tls-assets.uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you share the context behind this? AFAIK, removing explicit rkt rms just means to "defer" removals of rkt pods until rkt gc is run. Then, could there be a specific reason to do so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no particular reason, feels more right that way:

  • there were errors reported regarding rkt rm erroring
  • rkt-gc is already there,let it do its job

Copy link
Contributor

Choose a reason for hiding this comment

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

@redbaron Sorry missed the context on my side this time. I'd like to leave rkt rm as many as possible for system pods like decrypt-tls-assets to provide us chances to immediately notice issues like #199 (comment). Getting rid of rkt rms does defer but doesn't fix an issue like that.

I'm ok with rkt gc doing their jobs not on system pods(created via kube-aws) but rather user pods.

@mumoshu
Copy link
Contributor

mumoshu commented Jan 10, 2017

etcd decrypts assets differently from how it is done for controller and workers

@redbaron Thanks for pointing it out. Yes, and as it relates to #218, I'd like cloud-config-etcd to be updated similarly to the one for controllers/workers.

@mumoshu
Copy link
Contributor

mumoshu commented Jan 10, 2017

shouldn't it be an error if *.pem.enc files are not found? current behavior which I kept is to ignore their absence

@redbaron Good catch! Yes, I agree to you there. I'd like decrypt-tls-assets to emit errors when there're missing enc files.

@redbaron
Copy link
Contributor Author

Thanks for pointing it out. Yes, and as it relates to #218, I'd like cloud-config-etcd to be updated similarly to the one for controllers/workers.

I have a big change to the way etcd are provisioned, it is updated there. I didn't finish testing it, but can submit WIP if want to have a look

@mumoshu
Copy link
Contributor

mumoshu commented Jan 10, 2017

@redbaron Yes, I'd greatly appreciate your PR and am ready to review.

@redbaron
Copy link
Contributor Author

restored rkt rm but ignoring errors from it

@mumoshu
Copy link
Contributor

mumoshu commented Jan 11, 2017

@redbaron Thanks! Could you also resolve the conflicts? I guess taint-and-uncordon should be conflicted with #199 at least.

 - Use curl '--data-binary' to guarantee request body as-is
 - Use files by name directly instead of `$(cat)`
Due to use in ExecStartPre, there is a chance that multiple
copied of it can be run simultaneously
@mumoshu
Copy link
Contributor

mumoshu commented Jan 12, 2017

LGTM & E2E passed. Thanks as always @redbaron 👍 🙇

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants