You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/03/25 03:36:59 UTC

[GitHub] [incubator-yunikorn-k8shim] wilfred-s opened a new pull request #248: [YUNIKORN-600] PlaceholderManager structure init

wilfred-s opened a new pull request #248:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/248


   Fix the initialisation of the PlaceholderManager structure to init the
   channel and orphanPods map (renamed).
   Fix the tests to call NewPlaceholderManager to not init the structure in
   the test code and thus hiding init issues.
   
   The stop channel is never closed and should be reused. We should not
   create a new channel in the Start but in the New call. Fix up the Stop
   checks to not use one long 5 second timeout but check every 100ms. This
   change speeds up the test (down to 0.2s from 8s) and the shutdown of the
   whole shim.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao commented on a change in pull request #248: [YUNIKORN-600] PlaceholderManager structure init

Posted by GitBox <gi...@apache.org>.
HuangTing-Yao commented on a change in pull request #248:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/248#discussion_r601101370



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -220,27 +220,44 @@ func TestCleanOrphanPlaceholders(t *testing.T) {
 			UID:  "UID-01",
 		},
 	}
-	placeholderMgr.orphanPod["task01"] = pod1
-	assert.Equal(t, len(placeholderMgr.orphanPod), 1)
+	placeholderMgr.orphanPods["task01"] = pod1
+	assert.Equal(t, len(placeholderMgr.orphanPods), 1)
 	placeholderMgr.cleanOrphanPlaceholders()
-	assert.Equal(t, len(placeholderMgr.orphanPod), 0)
+	assert.Equal(t, len(placeholderMgr.orphanPods), 0)
 }
 
 func TestPlaceholderManagerStartStop(t *testing.T) {
 	mockedAPIProvider := client.NewMockedAPIProvider()
-	placeholderMgr := &PlaceholderManager{
-		clients:   mockedAPIProvider.GetAPIs(),
-		orphanPod: make(map[string]*v1.Pod),
-		running:   atomic.Value{},
-		RWMutex:   sync.RWMutex{},
-	}
-	placeholderMgr.setRunning(false)
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	assert.Equal(t, mgr.running.Load(), false, "new manager should not run")
 	// start clean up goroutine
-	placeholderMgr.Start()
-	assert.Equal(t, placeholderMgr.isRunning(), true)
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+
+	// starting a second time should do nothing
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "sending start 2nd time should not do anything")
+
+	// this is a blocking call
+	mgr.Stop()
+	// allow time for processing as the call can return faster than the flag is set
+	time.Sleep(5 * time.Millisecond)

Review comment:
       In `Statt()` it might sleep `100 * time.Millisecond` then check the stop chan, but here only sleep `5 * time.Millisecond`.
   I am not sure it is enough for it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao merged pull request #248: [YUNIKORN-600] PlaceholderManager structure init

Posted by GitBox <gi...@apache.org>.
HuangTing-Yao merged pull request #248:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/248


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao commented on a change in pull request #248: [YUNIKORN-600] PlaceholderManager structure init

Posted by GitBox <gi...@apache.org>.
HuangTing-Yao commented on a change in pull request #248:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/248#discussion_r601206380



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -220,27 +220,44 @@ func TestCleanOrphanPlaceholders(t *testing.T) {
 			UID:  "UID-01",
 		},
 	}
-	placeholderMgr.orphanPod["task01"] = pod1
-	assert.Equal(t, len(placeholderMgr.orphanPod), 1)
+	placeholderMgr.orphanPods["task01"] = pod1
+	assert.Equal(t, len(placeholderMgr.orphanPods), 1)
 	placeholderMgr.cleanOrphanPlaceholders()
-	assert.Equal(t, len(placeholderMgr.orphanPod), 0)
+	assert.Equal(t, len(placeholderMgr.orphanPods), 0)
 }
 
 func TestPlaceholderManagerStartStop(t *testing.T) {
 	mockedAPIProvider := client.NewMockedAPIProvider()
-	placeholderMgr := &PlaceholderManager{
-		clients:   mockedAPIProvider.GetAPIs(),
-		orphanPod: make(map[string]*v1.Pod),
-		running:   atomic.Value{},
-		RWMutex:   sync.RWMutex{},
-	}
-	placeholderMgr.setRunning(false)
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	assert.Equal(t, mgr.running.Load(), false, "new manager should not run")
 	// start clean up goroutine
-	placeholderMgr.Start()
-	assert.Equal(t, placeholderMgr.isRunning(), true)
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+
+	// starting a second time should do nothing
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "sending start 2nd time should not do anything")
+
+	// this is a blocking call
+	mgr.Stop()
+	// allow time for processing as the call can return faster than the flag is set
+	time.Sleep(5 * time.Millisecond)

Review comment:
       OK, got it. Thanks.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on pull request #248: [YUNIKORN-600] PlaceholderManager structure init

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #248:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/248#issuecomment-806355154


   @HuangTing-Yao  can you help to review this change? THX


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] HuangTing-Yao commented on a change in pull request #248: [YUNIKORN-600] PlaceholderManager structure init

Posted by GitBox <gi...@apache.org>.
HuangTing-Yao commented on a change in pull request #248:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/248#discussion_r601101370



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -220,27 +220,44 @@ func TestCleanOrphanPlaceholders(t *testing.T) {
 			UID:  "UID-01",
 		},
 	}
-	placeholderMgr.orphanPod["task01"] = pod1
-	assert.Equal(t, len(placeholderMgr.orphanPod), 1)
+	placeholderMgr.orphanPods["task01"] = pod1
+	assert.Equal(t, len(placeholderMgr.orphanPods), 1)
 	placeholderMgr.cleanOrphanPlaceholders()
-	assert.Equal(t, len(placeholderMgr.orphanPod), 0)
+	assert.Equal(t, len(placeholderMgr.orphanPods), 0)
 }
 
 func TestPlaceholderManagerStartStop(t *testing.T) {
 	mockedAPIProvider := client.NewMockedAPIProvider()
-	placeholderMgr := &PlaceholderManager{
-		clients:   mockedAPIProvider.GetAPIs(),
-		orphanPod: make(map[string]*v1.Pod),
-		running:   atomic.Value{},
-		RWMutex:   sync.RWMutex{},
-	}
-	placeholderMgr.setRunning(false)
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	assert.Equal(t, mgr.running.Load(), false, "new manager should not run")
 	// start clean up goroutine
-	placeholderMgr.Start()
-	assert.Equal(t, placeholderMgr.isRunning(), true)
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+
+	// starting a second time should do nothing
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "sending start 2nd time should not do anything")
+
+	// this is a blocking call
+	mgr.Stop()
+	// allow time for processing as the call can return faster than the flag is set
+	time.Sleep(5 * time.Millisecond)

Review comment:
       In `Start()` it might sleep `100 * time.Millisecond` then check the stop chan, but here only sleep `5 * time.Millisecond`.
   I am not sure it is enough for it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #248: [YUNIKORN-600] PlaceholderManager structure init

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #248:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/248#issuecomment-806342601


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248?src=pr&el=h1) Report
   > Merging [#248](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248?src=pr&el=desc) (10ea3ab) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **increase** coverage by `0.36%`.
   > The diff coverage is `62.50%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #248      +/-   ##
   ==========================================
   + Coverage   59.75%   60.11%   +0.36%     
   ==========================================
     Files          35       35              
     Lines        3133     3237     +104     
   ==========================================
   + Hits         1872     1946      +74     
   - Misses       1180     1211      +31     
   + Partials       81       80       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `74.40% <ø> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `45.00% <8.33%> (-8.07%)` | :arrow_down: |
   | [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `74.11% <68.00%> (-2.64%)` | :arrow_down: |
   | [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `63.15% <80.00%> (ø)` | |
   | ... and [9 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248?src=pr&el=footer). Last update [321ed13...10ea3ab](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/248?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #248: [YUNIKORN-600] PlaceholderManager structure init

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #248:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/248#discussion_r601184397



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -220,27 +220,44 @@ func TestCleanOrphanPlaceholders(t *testing.T) {
 			UID:  "UID-01",
 		},
 	}
-	placeholderMgr.orphanPod["task01"] = pod1
-	assert.Equal(t, len(placeholderMgr.orphanPod), 1)
+	placeholderMgr.orphanPods["task01"] = pod1
+	assert.Equal(t, len(placeholderMgr.orphanPods), 1)
 	placeholderMgr.cleanOrphanPlaceholders()
-	assert.Equal(t, len(placeholderMgr.orphanPod), 0)
+	assert.Equal(t, len(placeholderMgr.orphanPods), 0)
 }
 
 func TestPlaceholderManagerStartStop(t *testing.T) {
 	mockedAPIProvider := client.NewMockedAPIProvider()
-	placeholderMgr := &PlaceholderManager{
-		clients:   mockedAPIProvider.GetAPIs(),
-		orphanPod: make(map[string]*v1.Pod),
-		running:   atomic.Value{},
-		RWMutex:   sync.RWMutex{},
-	}
-	placeholderMgr.setRunning(false)
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	assert.Equal(t, mgr.running.Load(), false, "new manager should not run")
 	// start clean up goroutine
-	placeholderMgr.Start()
-	assert.Equal(t, placeholderMgr.isRunning(), true)
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+
+	// starting a second time should do nothing
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "sending start 2nd time should not do anything")
+
+	// this is a blocking call
+	mgr.Stop()
+	// allow time for processing as the call can return faster than the flag is set
+	time.Sleep(5 * time.Millisecond)

Review comment:
       This sleep time has nothing to do with the sleep in the PlaceholderManager. The sleep that is here is purely to allow the go routine to process updating the stop flag so we can see that it has been done.
   The `stopChan` is an unbuffered channel. The `mgr.Stop()` call is thus blocking until the channel has been read. The channel read happens before the flag gets set that is why we need to sleep in the test. The atomic update of the flag must be done before we check. Without the small sleep in the test the go routine might not have done that on a multi core machine when the test progresses.
   
   You can see that the call is blocking when you run the old code and the test. It takes 8s to complete. When I replaced the `time.Sleep(3 * time.Second)` in the old test with `time.Sleep(5 * time.Millisecond)` the test finished in 5s. Similar when I changed the clean interval the test ran as long as the clean interval was set to.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org