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

fix(controller): Adds PNS_PRIVILEGED, fixed termination bug #4983

Merged
merged 3 commits into from Feb 1, 2021

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Feb 1, 2021

I've removed the need to run ALL tests.

New suites:

  • ArtifactsSuite - tests around artifacts
  • SignalsSuite - test related to signals sent to executor (e.g. SIGTEM)

I've deleted a few tests (see my comments).

Fixed a bug that marked some workflows as an error when they should have been failed.

Added PNS_PRIVILEGED to run PNS in privileged mode, sometimes needed for GCP.

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec changed the title test: More tests test: Group e2e tests Feb 1, 2021
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec changed the title test: Group e2e tests fix(controller): Adds PNS_PRIVILEGED, fixed termination bug Feb 1, 2021
@@ -293,25 +281,6 @@ spec:
})
}

func (s *FunctionalSuite) TestFastFailOnPodTermination() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted as skipped

@@ -554,87 +523,6 @@ func (s *FunctionalSuite) TestParameterAggregation() {
})
}

func (s *FunctionalSuite) TestGlobalScope() {
s.Need(fixtures.BaseLayerArtifacts)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted - this test is flakey and it was a time suck fixing it

@@ -1116,38 +1116,29 @@ func (woc *wfOperationCtx) assessNodeStatus(pod *apiv1.Pod, node *wfv1.NodeStatu
}
newDaemonStatus = pointer.BoolPtr(false)
case apiv1.PodRunning:
if pod.DeletionTimestamp != nil {
Copy link
Contributor Author

@alexec alexec Feb 1, 2021

Choose a reason for hiding this comment

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

this condition is incorrect - a pod with a deletion timestamp is one we may have caused to be deleted ourselves - this was incorrect marking nodes as error when they should have been failed

@alexec alexec marked this pull request as ready for review February 1, 2021 21:26
@alexec alexec merged commit b68d63e into argoproj:master Feb 1, 2021
@alexec alexec deleted the more-tests branch February 1, 2021 23:32
@alexec alexec linked an issue Feb 2, 2021 that may be closed by this pull request
@simster7 simster7 mentioned this pull request Feb 8, 2021
38 tasks
@lnguyen
Copy link

lnguyen commented Mar 3, 2021

can this be cherry picked for v2.x?

@alexec
Copy link
Contributor Author

alexec commented Mar 3, 2021

This is too big to back-port all of it really. Is there a specific thing you need from it?

@lnguyen
Copy link

lnguyen commented Mar 3, 2021

will it be easy transition to v3.x? no specific need, just running this using onepanel on microk8s with pns and getting alot of crashes

@alexec
Copy link
Contributor Author

alexec commented Mar 3, 2021

Most user should not have any migration issues to v3.0

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.

Experiencing issues with the PNS executor
3 participants