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

Add multi-namespace cache support #1995

Merged
merged 3 commits into from
Oct 17, 2019

Conversation

charith-elastic
Copy link
Contributor

@charith-elastic charith-elastic commented Oct 15, 2019

Uses the multi-namespace cache if the operator is provided with a list of namespaces to be managed. This would allow users to restrict the operator to a small set of namespaces.

For backward compatibility reasons, namespace flag has been retained but it will be ignored if the namespace-list flag is provided.

Also changes the E2E test runner to have a single operator for all E2E namespaces rather than an operator per namespace.

Fixes #405, #782

@charith-elastic charith-elastic added the >enhancement Enhancement of existing functionality label Oct 15, 2019
@anyasabo
Copy link
Contributor

I know in the past we discussed maybe wanting two separate flags since we chose the singular namespace. I think because we are still in beta it would be worthwhile to have the breaking change and only support namespaces. That way we do not need to have the potentially confusing mix of namespace and namespaces with one taking priority over the other, and we also have less code. I'd also rather do breaking changes sooner than later so we have more time to iron out any issues. What do you think?

Makefile Outdated Show resolved Hide resolved
@@ -33,7 +33,7 @@ spec:
containers:
- name: e2e
image: {{ .E2EImage }}
imagePullPolicy: IfNotPresent
imagePullPolicy: Always
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with how E2EImage was populated. Were we reusing image names across test runs and so they were getting cached even if they changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Image caching is a problem if you manually run the tests with uncommitted changes.

I kept the setting because it won't affect the normal CI runs (E2E image name is unique and will require a pull from the registry anyway).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may have introduced IfNotPresent a while ago in order to run tests on Minikube. We didn't have a registry there, so we used to build the image in Minikube context. Meaning it was possible to use it, but not to pull it from Minikube.
The situation is different now, we do run a registry in Minikube if I remember correctly (hack/registry.sh).

@charith-elastic
Copy link
Contributor Author

I agree with @anyasabo about not having both namespace and namespace-list in the long term. My thinking is that we should keep both flags until the GA release is confirmed, and then drop the namespace flag and (maybe) rename namespace-list to namespaces.

test/e2e/cmd/run/run.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
@@ -33,7 +33,7 @@ spec:
containers:
- name: e2e
image: {{ .E2EImage }}
imagePullPolicy: IfNotPresent
imagePullPolicy: Always
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may have introduced IfNotPresent a while ago in order to run tests on Minikube. We didn't have a registry there, so we used to build the image in Minikube context. Meaning it was possible to use it, but not to pull it from Minikube.
The situation is different now, we do run a registry in Minikube if I remember correctly (hack/registry.sh).

config/e2e/namespace_operator.yaml Show resolved Hide resolved
@sebgl
Copy link
Contributor

sebgl commented Oct 16, 2019

Regarding breaking changes: I think I would also vote for removing the singular namespace right now.

Arguments:

  • Most users will probably upgrade to next release by applying the newer operator yaml resource, so they won't notice.
  • If they apply the new operator manifest with a singular namespace, I think the error will be explicit in the operator pod logs.
  • Doing it in 2-phases as @charith-elastic suggested sounds like the right way to deal with api changes in general. In this particular case I feel like it's not strictly required though (it's not "really" the public user API), and will actually be a breaking change in 2 releases (post-GA, once we remove the deprecated flag). Might as well break it now IMO.

Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

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

LGTM

I agree with the small breakage since we are the ones who ship the operator yaml.

@charith-elastic charith-elastic merged commit 81aee4a into elastic:master Oct 17, 2019
@charith-elastic charith-elastic deleted the multi-namespace-cache branch October 17, 2019 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >enhancement Enhancement of existing functionality v1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multi-namespace watches
5 participants