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

Reconcile noobaa pods HTTP* envs based on operator pod HTTP* envs #313

Merged
merged 1 commit into from May 31, 2020

Conversation

nb-ohad
Copy link
Contributor

@nb-ohad nb-ohad commented May 26, 2020

When running in a proxied environment the HTTP_PROXY, HTTPS_PROXY, and NO_PROXY environment variables control which of a pod's outgoing traffic is directed to a proxy and which not.

When cluster-wide proxy setting is used OLM update the operator's env variables to the proper values. In turn, the operator should set the environment variables on the resources that it is responsible for.

This PR update the operator to read the values of the HTTP* environment variables set on the operator container and set them for the noobaa-core statefulset and nobaa-endpoint deployments

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1819549

Edit:
We have a gap where changes to the env vars will not be reconciled for pv pool agent pods.
This is because a pod spec cannot be changed and we need takedown the pod and start a new one which we currently do not support with pv pool pods.

@nb-ohad nb-ohad requested a review from dannyzaken May 26, 2020 13:27
Copy link
Contributor

@dannyzaken dannyzaken left a comment

Choose a reason for hiding this comment

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

@jackyalbo merged the pv-pools PR. apply the proxy settings to pv-pools pods too

@nb-ohad nb-ohad requested a review from jackyalbo May 27, 2020 16:40
"IO_ERRORS": nbv1.BackingStorePhaseInfo{nbv1.BackingStorePhaseRejected, "BackingStorePhaseRejected", "Backing store mode: IO_ERRORS"},
"STORAGE_NOT_EXIST": nbv1.BackingStorePhaseInfo{nbv1.BackingStorePhaseRejected, "BackingStorePhaseRejected", "Backing store mode: STORAGE_NOT_EXIST"},
"AUTH_FAILED": nbv1.BackingStorePhaseInfo{nbv1.BackingStorePhaseRejected, "BackingStorePhaseRejected", "Backing store mode: AUTH_FAILED"},
"INITIALIZING": {nbv1.BackingStorePhaseReady, "BackingStorePhaseReady", "Backing store mode: INITALIZING"},
Copy link
Contributor

Choose a reason for hiding this comment

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

??
is that just a formatter thing? if it is, then we must align on the same rules so we won't have irrelevant changes in PRs

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, Ohad please revert

Copy link
Contributor Author

@nb-ohad nb-ohad May 28, 2020

Choose a reason for hiding this comment

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

The new GoFmt is removing unnecessary type declaration. We all use the same GoFmt and it runs automatically on every save

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an update. It's not the GoFmt version. It the gofmt settings. My editor is running gofmt with the simplify code flag while vscode does not (by default). I will try to check if I can disable the simplify behavior to be more consistent but as a rule of thumb I think this behavior can benefit us

Copy link
Contributor

@dannyzaken dannyzaken left a comment

Choose a reason for hiding this comment

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

after a second look at the code, I think you should not define the proxy vars ahead of time in the yamls. just test for the envs using LookupEnv and if they are set then append them to the Env of the different pods

@nb-ohad
Copy link
Contributor Author

nb-ohad commented May 28, 2020

@dannyzaken Why? This is the way we handle all of our env, even the ones that are not used in some circumstances. I see no benefit in doing it the other way around. Declaring them ahead of time give the reader a sense that understanding of what we are going to reconcile.
And functionally it behaves the same.

@dannyzaken
Copy link
Contributor

@nb-ohad are you sure it behaves the same? these envs are not used by noobaa, you are just propagating them from the operator to other pods. I think that in this case, you should forward them as is

@nb-ohad
Copy link
Contributor Author

nb-ohad commented May 31, 2020

@ALL
Please read the update at the end of PR description

pkg/backingstore/reconciler.go Outdated Show resolved Hide resolved
also warn when agent pod PROXY vars does not operator PROXY vars
@nb-ohad nb-ohad merged commit 12ad04c into noobaa:master May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants