dind: Ensure unique node config and fix perms #11326
Conversation
|
[test] |
|
This fixes more than just permissions. Previously node configuration was always generated into the same path on the shared volume - |
marun
force-pushed
the
dind-fix-config-perms
branch
from
18c5159
to
0aa77ee
Compare
marun
changed the title
| find "${config_path}" -exec chmod ga+rw {} \; | ||
| find "${config_path}" -type d -exec chmod ga+x {} \; | ||
| # ensure the configuration can be used outside of the container | ||
| chmod ga+rx "${config_path}" |
There was a problem hiding this comment.
Can we collapse this:
chmod -R ga+rX "${config_path}"
chmod ga+w "${master_path}/admin.kubeconfig"
marun
force-pushed
the
dind-fix-config-perms
branch
from
0aa77ee
to
e598327
Compare
|
|
marun
force-pushed
the
dind-fix-config-perms
branch
from
e598327
to
23beb2f
Compare
|
@stevekuznetsov Ok, I'll close the other one. I tend to try to split things up but these fixes are very closely related. |
| @@ -51,6 +58,9 @@ function ensure-node-config() { | |||
| --signer-cert="${master_config_path}/ca.crt" \ | |||
| --signer-key="${master_config_path}/ca.key" \ | |||
| --signer-serial="${master_config_path}/ca.serial.txt" | |||
|
|
|||
| # Release the lock | |||
| flock -o | |||
There was a problem hiding this comment.
flock(1) is so weird... pretty sure this is wrong though. At the least it would need a "200" there so it knows what to unlock? And I think you want -u not -o too.
I think the recommended way of using it is to scope it with a subshell:
(flock 200;
/usr/local/bin/openshift ...
) 200>/path/to/lockfile
so then the fd is closed automatically when leaving the subshell.
marun
force-pushed
the
dind-fix-config-perms
branch
from
23beb2f
to
c9fb01c
Compare
The cert gen commands are not intended to be run concurrently.
|
lgtm i think. let's see what the tests say... |
|
flake #8571 re-[test] |
|
@danwinship well, the check job passed. shame about the others that have nothing to do with this PR. |
|
flake #8571 re-[test] |
|
@openshift/networking The flakes are unrelated, please merge. |
|
flake #8571 re-[test] |
|
[merge] |
|
Evaluated for origin merge up to c9fb01c |
|
holy merge queue, batman |
|
Flake #10663 [merge] |
|
@knobunc the test job is running and failing, the merge has not yet occurred. I don't think re-triggering a merge will shove you to the end of the line, but it may be ... no need to re-tag |
|
@stevekuznetsov Thanks, I wasn't sure if the test fail was going to prevent the merge. |
today the relationship is one-way in that successful tests (against a master that hasn't changed) can help you, but failed tests can never hurt you |
|
[test] curious to see the networking job runtime |
|
Evaluated for origin test up to c9fb01c |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10059/) (Base Commit: 098e0ee) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10072/) (Base Commit: 0a022ab) (Image: devenv-rhel7_5181) |
|
lgtm |
A dind deployment flake on #11315 revealed that config permissions are not being set correctly to allow cluster artifacts to be saved. This change ensures the cluster configuration is readable so it can be saved.
cc: @openshift/networking @stevekuznetsov
Should also fix #11274