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): Fix active pods count in node pending status with pod deleted. #5836

Merged
merged 9 commits into from
May 10, 2021

Conversation

sarabala1979
Copy link
Member

@sarabala1979 sarabala1979 commented May 5, 2021

Checklist:

closes #5811

Signed-off-by: Saravanan Balasubramanian <[email protected]>
Signed-off-by: Saravanan Balasubramanian <[email protected]>
Signed-off-by: Saravanan Balasubramanian <[email protected]>
Signed-off-by: Saravanan Balasubramanian <[email protected]>
Signed-off-by: Saravanan Balasubramanian <[email protected]>
@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #5836 (80d1da1) into master (3f80866) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5836      +/-   ##
==========================================
+ Coverage   47.20%   47.36%   +0.16%     
==========================================
  Files         246      247       +1     
  Lines       15520    15595      +75     
==========================================
+ Hits         7326     7387      +61     
  Misses       7280     7280              
- Partials      914      928      +14     
Impacted Files Coverage Δ
pkg/apis/workflow/v1alpha1/workflow_types.go 46.53% <ø> (+0.48%) ⬆️
workflow/controller/operator.go 70.53% <ø> (-0.61%) ⬇️
workflow/controller/node_counters.go 100.00% <100.00%> (ø)
server/workflow/workflow_server.go 40.45% <0.00%> (-2.28%) ⬇️
util/file/fileutil.go 64.00% <0.00%> (-1.22%) ⬇️
workflow/validate/validate.go 72.98% <0.00%> (-0.41%) ⬇️
cmd/argo/commands/get.go 57.92% <0.00%> (ø)
cmd/argo/commands/submit.go 0.00% <0.00%> (ø)
pkg/apiclient/http1-client.go 0.00% <0.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f80866...80d1da1. Read the comment docs.

Signed-off-by: Saravanan Balasubramanian <[email protected]>
Signed-off-by: Saravanan Balasubramanian <[email protected]>
@sarabala1979 sarabala1979 marked this pull request as ready for review May 7, 2021 22:52
go.mod Outdated
@@ -24,6 +24,7 @@ require (
github.com/evanphx/json-patch v4.9.0+incompatible
github.com/fatih/structs v1.1.0 // indirect
github.com/gavv/httpexpect/v2 v2.2.0
github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the sig yaml library

p.yaml Outdated
@@ -0,0 +1,86 @@
apiVersion: v1
Copy link
Contributor

Choose a reason for hiding this comment

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

please delete this file (you can delete wft.yaml while you're at it)

podObjs = append(podObjs, pods.Items...)
for _, pod := range podObjs {
err = woc.controller.podInformer.GetIndexer().Add(&pod)
fmt.Println(err)
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 this should panic?

workflow/controller/node_counters.go Outdated Show resolved Hide resolved
workflow/controller/node_counters_test.go Outdated Show resolved Hide resolved
workflow/controller/node_counters_test.go Show resolved Hide resolved
workflow/controller/node_counters_test.go Outdated Show resolved Hide resolved
@@ -1395,6 +1395,8 @@ func TestWorkflowParallelismLimit(t *testing.T) {
assert.Equal(t, 2, len(pods.Items))
// operate again and make sure we don't schedule any more pods
makePodsPhase(ctx, woc, apiv1.PodRunning)
syncPodsInformer(ctx, woc)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion - could makePodsPhase call syncPodsInformer?

Signed-off-by: Saravanan Balasubramanian <[email protected]>
Signed-off-by: Saravanan Balasubramanian <[email protected]>
@sarabala1979 sarabala1979 merged commit 2687e24 into argoproj:master May 10, 2021
@sarabala1979 sarabala1979 mentioned this pull request May 13, 2021
35 tasks
@alexec alexec mentioned this pull request May 21, 2021
14 tasks
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.

Parameter loop fails with pod deleted
2 participants