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

DEVEXP-424: Mounting node pull secrets #284

Merged
merged 1 commit into from Apr 9, 2020
Merged

DEVEXP-424: Mounting node pull secrets #284

merged 1 commit into from Apr 9, 2020

Conversation

ricardomaraschini
Copy link
Contributor

@ricardomaraschini ricardomaraschini commented Dec 18, 2019 •
edited

OpenShift API Server needs to make use of node pull credentials during image stream import. This PR mounts them inside the operand pods. For further details please refer to the enhancement proposal.

Note: Please review openshift/openshift-apiserver#83 as well

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 18, 2019
@ricardomaraschini
Copy link
Contributor Author

/test e2e-aws

@ricardomaraschini
Copy link
Contributor Author

/assign @dmage @adambkaplan

@dmage
Copy link
Member

dmage commented Dec 19, 2019

Support of these pull secrets will be added to the API server by openshift/openshift-apiserver#64.

I doubt about the prefix /node, I would remove it.

@mfojtik can you take a look?

@ricardomaraschini
Copy link
Contributor Author

I doubt about the prefix /node, I would remove it.

I am also not happy with it tbh. What would be a good location in your opinion?
All mount points seems to use /var/run/ as prefix, maybe /var/run/node ?

@ricardomaraschini
Copy link
Contributor Author

/assign @mfojtik

@adambkaplan adambkaplan mentioned this pull request Jan 6, 2020
Closed
@dmage
Copy link
Member

dmage commented Jan 23, 2020

/cc @deads2k

adambkaplan
adambkaplan approved these changes Jan 23, 2020
View reviewed changes
Copy link

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/lgtm

Mount path is in accordance with openshift/enhancements#136

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@ricardomaraschini
Copy link
Contributor Author

/retest

@sttts
Copy link
Contributor

sttts commented Jan 24, 2020

/hold

until proposal is merged.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2020
@sttts sttts mentioned this pull request Jan 24, 2020
Merged
@dmage
Copy link
Member

dmage commented Jan 29, 2020

/retitle DEVEXP-424: Mounting node pull secrets

@openshift-ci-robot openshift-ci-robot changed the title DEVEXP 424: Mounting node pull secrets. DEVEXP-424: Mounting node pull secrets Jan 29, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2020
@ricardomaraschini
Copy link
Contributor Author

/unhold

enhancement proposal was merged.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2020
@ricardomaraschini
Copy link
Contributor Author

/retest

@ricardomaraschini
Mounting node pull secrets.
008fe65 
OpenShift API Server needs to make use of node pull credentials. This PR
mounts them inside the operand pod.
@gabemontero gabemontero mentioned this pull request Mar 18, 2020
Merged
@ricardomaraschini ricardomaraschini mentioned this pull request Mar 18, 2020
Merged
adambkaplan
adambkaplan approved these changes Mar 25, 2020
View reviewed changes
Copy link

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2020
@adambkaplan
Copy link

/assign @mfojtik

/cc @sttts

@mfojtik
Copy link
Member

mfojtik commented Mar 26, 2020

@adambkaplan @ricardomaraschini i assume the config.json is not something new that was added in last release and we don't have to solve a case where that file is missing, right?

Otherwise this change looks fine to me, the reloading should be handled by node reboot, which seems to be sufficient.

@adambkaplan
Copy link

i assume the config.json is not something new that was added in last release and we don't have to solve a case where that file is missing, right?

@mfojtik correct - this is the cluster pull secret that MCO (or something in that area) has been adding to the node for quite some time.

@dmage
Copy link
Member

dmage commented Apr 7, 2020

@mfojtik can you tag it?

@sttts
Copy link
Contributor

sttts commented Apr 9, 2020

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, ricardomaraschini, sttts

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 480533b into openshift:master Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adambkaplan adambkaplan adambkaplan approved these changes

@mfojtik mfojtik Awaiting requested review from mfojtik

@deads2k deads2k Awaiting requested review from deads2k

@sttts sttts Awaiting requested review from sttts

Assignees

@mfojtik mfojtik

@dmage dmage

@adambkaplan adambkaplan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ricardomaraschini @dmage @sttts @adambkaplan @mfojtik @openshift-ci-robot @openshift-bot @openshift-merge-robot