Add integration tests for the instance admission controller #1442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
The `should clear scheduled plan when it is completed` test is failing: this is a bug in the IAC that needs to be fixed. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
introduced new `PlanExecution.Status` field which is written by IC and read by IAC signalling that a plan is terminal. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
zen-dog
added
the
release/breaking-change
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
zen-dog
requested review from
alenkacz,
gerred,
kensipe and
nfnt
as code owners
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
This was referenced Apr 1, 2020
zen-dog
removed
the
release/breaking-change
kensipe
reviewed
Apr 2, 2020
| @@ -9,6 +9,8 @@ require ( | |||
| github.com/gosuri/uitable v0.0.4 | |||
| github.com/kudobuilder/kuttl v0.1.0 | |||
| github.com/manifoldco/promptui v0.6.0 | |||
| github.com/onsi/ginkgo v1.11.0 | |||
| github.com/onsi/gomega v1.8.1 | |||
There was a problem hiding this comment.
I thought we previously had these as dependencies and they were removed... now they are back?
I'm not against... but now find myself curious if there are competing mental models
There was a problem hiding this comment.
Unit tests do not need it and neither does kuttl. However, instance_admission_integration_test is complex enough that ginkgo becomes necessary. Gomega is not strictly necessary and we could use an existing matcher lib but both seem to be used together everwhere else so I didn't want to break that pattern 🤷♂
| @@ -183,7 +184,7 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion | |||
| isPlanRetriggered := hadPlan && newPlan == oldPlan && newUID != oldUID | |||
| isPlanCancellation := hadPlan && newPlan == "" | |||
| isDeleting := new.IsDeleting() // a non-empty meta.deletionTimestamp is a signal to switch to the uninstalling life-cycle phase | |||
| isPlanTerminal := isTerminal(new, newPlan, new.Spec.PlanExecution.UID) | |||
| isPlanTerminal := new.Spec.PlanExecution.Status.IsTerminal() | |||
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
kensipe
approved these changes
Apr 2, 2020
zen-dog
added a commit
that referenced
this pull request
Apr 17, 2020
Summary: #1442 introduced new `Instance.Spec.PlanExecution.Status` field which duplicates part of the `Instance.Spec.PlanStatus` information . With that, plan execution status is now saved three times: in `planExecution`, `aggregatedStatus` and the corresponding `planStatus` fields e.g.: ```yaml apiVersion: kudo.dev/v1beta1 kind: Instance spec: planExecution: planName: deploy status: IN_PROGRESS status: aggregatedStatus: activePlanName: deploy status: IN_PROGRESS planStatus: deploy: name: deploy phases: - name: deploy status: IN_PROGRESS steps: - name: app status: IN_PROGRESS status: IN_PROGRESS ``` `aggregatedStatus` is completely duplicating `planExecution` while `planStatus` provides extended information on individual phases and steps. This PR removes `aggregatedStatus` field in favour of `planExecution`. The only caveat is that we previously kept `aggregatedStatus.status` value even **after** the plan was finished to signal how _last_ plan execution ended. However, `kudo plan status` already calculates and displays this information from the corresponding `planStatus`. Additionally, having `aggregatedStatus.activePlanName` populated when no plan is active is somewhat broken to begin with. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
zen-dog
added a commit
that referenced
this pull request
Apr 20, 2020
Summary: #1442 introduced new `Instance.Spec.PlanExecution.Status` field which duplicates part of the `Instance.Spec.PlanStatus` information . With that, plan execution status is now saved three times: in `planExecution`, `aggregatedStatus` and the corresponding `planStatus` fields e.g.: ```yaml apiVersion: kudo.dev/v1beta1 kind: Instance spec: planExecution: planName: deploy status: IN_PROGRESS status: aggregatedStatus: activePlanName: deploy status: IN_PROGRESS planStatus: deploy: name: deploy phases: - name: deploy status: IN_PROGRESS steps: - name: app status: IN_PROGRESS status: IN_PROGRESS ``` `aggregatedStatus` is completely duplicating `planExecution` while `planStatus` provides extended information on individual phases and steps. This PR removes `aggregatedStatus` field in favour of `planExecution`. The only caveat is that we previously kept `aggregatedStatus.status` value even **after** the plan was finished to signal how _last_ plan execution ended. However, `kudo plan status` already calculates and displays this information from the corresponding `planStatus`. Additionally, having `aggregatedStatus.activePlanName` populated when no plan is active is somewhat broken to begin with. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
zen-dog
pushed a commit
that referenced
this pull request
Apr 30, 2020
Summary: #1442 introduced new `Instance.Spec.PlanExecution.Status` field which duplicates part of the `Instance.Spec.PlanStatus` information . With that, plan execution status is now saved three times: in `planExecution`, `aggregatedStatus` and the corresponding `planStatus` fields e.g.: ```yaml apiVersion: kudo.dev/v1beta1 kind: Instance spec: planExecution: planName: deploy status: IN_PROGRESS status: aggregatedStatus: activePlanName: deploy status: IN_PROGRESS planStatus: deploy: name: deploy phases: - name: deploy status: IN_PROGRESS steps: - name: app status: IN_PROGRESS status: IN_PROGRESS ``` `aggregatedStatus` is completely duplicating `planExecution` while `planStatus` provides extended information on individual phases and steps. This PR removes `aggregatedStatus` field in favor of `planExecution`. The only caveat is that we previously kept `aggregatedStatus.status` value even **after** the plan was finished to signal how _last_ plan execution ended. However, `kudo plan status` already calculates and displays this information from the corresponding `planStatus`. Additionally, having `aggregatedStatus.activePlanName` populated when no plan is active is somewhat broken, to begin with. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
the tests discovered a bug in the instance admission controller (IAC) where IAC was trying to reset
PlanExecution.PlanNamewhen a plan becomes terminal. However, it wasn't doing this because:Instance.Statuswas being updated (Status is a sub-resource) andInstanceresource can not be changed uponStatussubresource updates (any resource is only allowed to modify itself from the webhook)To fix this problem we would either need to merge
Statusback into theInstanceresource or duplicate part of theStatusinformation in theInstance.Spec. After long discussions, we decided to introduce a newPlanExecution.Statusfield which is populated by the instance controller and duplicates the correspondingStatus.PlanStatusfield.