-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Check for circular dependencies in buildConfigs #1468
Conversation
Eventually I think I will have to move all this code into the buildConfig registry as there I will have total control over the updates and deletions of buildConfigs. |
circular dependencies aren't necessarily bad. you might be doing a test / validation flow and reusing input from the old image. I think the way we would implement openshift build on openshift is via a circular build. |
what we're trying to avoid here is buildA outputting imageA which triggers buildB which outputs imageB which triggers buildA. that's definitely bad, the build loop would never end. |
I'm saying there are use cases for that. The second build might block waiting for an upstream dependency and then manually trigger the base image. We also will eventually have squash images. The tool should flag them but they're not (yet) verboten.
|
if you were doing the openshift on openshift scenario you describe, i'd expect you to be differentiating by tags. A config like: The idea that you might have an intentional chain that's "broken" by having a manual step somewhere in there is interesting, i guess i can see the argument for that though i still think differentiating on tags seems like the more likely way to handle flowing images through test/validation steps. |
Probably. Andy has been working on the proposals here so once we know more there we can come back to circularity.
|
I have added simple warnings instead of erroring for the time being, but I share the same thoughts as @bparees, we shouldn't allow circular deps by default. If there are any use-cases, we should probably add an option of enabling/disabling them (still the default should be not supporting them). |
There's no way to prevent circular dependencies in OpenShift without a lot of overhead. So it's likely there will always be circular dependencies and we should be able to handle them gracefully and warn users.
|
I have moved the code under the |
Added tests and they do pass:) |
deps[from] = make(map[string]string) | ||
} | ||
fromDeps := deps[from] | ||
fromDeps[to] = config.Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to include the config namespace.
rather than maintaining an in memory datastructure (which is problematic since you're not rebuilding it at startup), i'd rather see this just use the same logic as the CLI build-chain tool. Can't that share some logic here? Just produce the build tree for the ICT From imagerepo, checking for cycles as you go. you'll want that check in the CLI tool logic anyway since w/o it, if you invoke the CLI tool on a tree w/ cycles it'll never terminate, so it would be good to abort with an error message when it hits cycle. |
@@ -75,6 +77,11 @@ func (r *REST) Create(ctx kapi.Context, obj runtime.Object) (runtime.Object, err | |||
if errs := validation.ValidateBuildConfig(buildConfig); len(errs) > 0 { | |||
return nil, errors.NewInvalid("buildConfig", buildConfig.Name, errs) | |||
} | |||
|
|||
if circularDeps(buildConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, we should not be adding this. This is way too expensive to calculate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And by that I mean we have to run an O(N) load on the cluster for every create, which is extremely expensive. Ben and I talked through a number of options, but none of them are really efficient or easy to implement, and they don't solve the problem for end users.
At a minimum, osc status
will show some of this for end users and that will reduce the possibility this happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running O(N) loads in maps of maps is expensive, agreed, but I suspect this can be refactored to use a cheaper structure where it won't really matter.
@bparees so what you are suggesting is rebuilding the tree every time? Because in the build-chain tool we just build the tree and are done. Here we need to maintain one (add nodes, update nodes, delete nodes) which is different and more challenging. |
@Kargakis you can't really maintain a universal structure anyway since visibility rules could change, for example (A->B->C->A, previously I didn't have permission to view C so it wasn't a cycle, suddenly I get permissions added, which creates a cycle). So you'd have to rebuild your structure whenever visibility rules changed, at a minimum. In general there seemed to be a lot of challenges here, and @smarterclayton convinced me the right thing to w/ respect to cycles is:
|
FWIW that's not true. The whole structure nevers needs to be rebuilt because every time we add or remove a relationship, we traverse all related nodes. |
@Kargakis the visibility rules changing is not an add/removal of a relationship. |
We should not do this as part of API calls (it violates our O(1) API design goals for complexity of API ops). Let's defer this until post 3.0. ----- Original Message -----
|
Agreed. Closing this... |
…service-catalog/' changes from aa27078754..510060232e 510060232e origin build: add origin tooling de45e94 v0.1.0 chart changes (openshift#1468) 0bb9982 Modify Makefile to only specify ldflags once (openshift#1471) 5d6afac Fixes openshift#735: Add repo-sync script for charts (openshift#1453) 630f13f fix lingering unversioned client API (openshift#1466) 6f49128 Fix several logging errors (openshift#1464) 2aece61 Delete removed serviceClasses when they have no instances left (openshift#1450) 179d302 Uncommenting UID field after updating to k8s 1.8 (openshift#1457) b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459) 4bea012 Use versioned client APIs (openshift#1458) ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452) 3fddf27 clean up logic and fix message for failed poll (openshift#1451) 40926cd Fix typo from openshift#1354 (openshift#1456) ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444) 8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448) ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343) 7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439) baf28de Create listers before adding event handlers in controller (openshift#1446) 294157d remove setServiceBindingCondition dependency on controller (openshift#1441) 118a0f7 Fix typo in validation (openshift#1447) 117bfbd clean up error logging (openshift#1443) dff470f Move "External" around in some resource names/properties (openshift#1354) 0885edb Adding expectedGot function and using it. (openshift#1440) a7d582e Pretty controller broker (openshift#1442) c5edfaf Set apimachinery build variables with semver info (openshift#1429) 0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408) fb874df Remove deprecated basic auth config support (openshift#1431) f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433) 96b286e Make service/plan reference fields on instance spec selectable (openshift#1422) 33f2b04 First example using the pretty context builder. (openshift#1403) 7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417) fcf9480 Add tests for plan updates (openshift#1412) 819332e Add root CAs (openshift#1419) b49a76a Clean Makefile a little (openshift#1399) d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415) 314a622 Wire etcd prefix to storage and call complete with options (openshift#1394) REVERT: aa27078754 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 510060232e54eb64b294213bb5d7847e169a2fac
No description provided.