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

Only retain last state for condition types #715

Merged
merged 1 commit into from Mar 28, 2022

Conversation

vthapar
Copy link
Contributor

@vthapar vthapar commented Mar 24, 2022

Condition types are expected to be singelton and only last
state of a given condition type shoud be maintained. We currently
maintain a list of all transitions to a state as a list with max of
10. This causes problems with condition check APIs. Currently
support just one type, Valid.

Fixes #714

Signed-off-by: Vishal Thapar 5137689+vthapar@users.noreply.github.com

Condition types are expected to be singelton and only last
state of a given condition type shoud be maintained. We currently
maintain a list of all transitions to a state as a list with max of
10. This causes problems with condition check APIs. Currently
support just one type, Valid.

Fixes submariner-io#714

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>
@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr715/vthapar/unique-conditions
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@@ -410,20 +408,14 @@ func (a *Controller) updateExportedServiceStatus(name, namespace string, status
Message: &msg,
}

numCond := len(toUpdate.Status.Conditions)
if numCond > 0 && serviceExportConditionEqual(&toUpdate.Status.Conditions[numCond-1], &exportCondition) {
// TODO: Currently we only check for conditionType Valid. Revisit this when Conflict is supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

To preserve the equivalent of the historical trace, we could add other condition types like "SyncedToBroker". But we can look into that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should, but we will have to propose these to MCS sig first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't think so. The MCS API defines some common, general ones but I'd think an implementation should be able to define their own specific types - the MCS spec doesn't preclude that. Eg, the type "SyncedToBroker" would be specific to our implementation.

@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Mar 28, 2022
@sridhargaddam sridhargaddam enabled auto-merge (rebase) March 28, 2022 06:51
@sridhargaddam sridhargaddam merged commit 2b66c0c into submariner-io:devel Mar 28, 2022
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr715/vthapar/unique-conditions]

@dfarrell07
Copy link
Member

@vthapar Do you agree this should be backported? Do you want to send it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport backport-handled ready-to-test When a PR is ready for full E2E testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Submariner ServiceExport does not have unique condition types to wait for readiness
5 participants