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 2022/09/27 00:29:17 UTC
[GitHub] [yunikorn-k8shim] craigcondit opened a new pull request, #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
craigcondit opened a new pull request, #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466
### What is this PR for?
Update build dependencies to Kubernetes 1.23.12. This provides better support for newer Kubernetes releases. However, this will drop support for running on Kubernetes releases earlier than 1.21.
### What type of PR is it?
* [ ] - Bug Fix
* [x] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1196
### How should this be tested?
E2e tests are provided and test matrix has been updated.
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [x] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#discussion_r1018633647
##########
deployments/image/plugin/start-yunikorn-plugin.sh:
##########
@@ -17,13 +17,15 @@
# limitations under the License.
cd "${HOME}" || exit
+
+# Embed scheduler name into config file
+sed 's/schedulerName:.*/schedulerName: '"${SCHEDULER_NAME}"'/' < "${SCHEDULER_CONFIG}" > /tmp/scheduler-config.yaml || exit
Review Comment:
We need to move this out of this shell script and into pre-processing as this requires `sed` to be installed in the image which might not be the case for stripped down setups.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s closed pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
URL: https://github.com/apache/yunikorn-k8shim/pull/466
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] craigcondit commented on a diff in pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#discussion_r1023406758
##########
test/e2e/recovery_and_restart/recovery_and_restart_test.go:
##########
@@ -171,9 +171,15 @@ var _ = ginkgo.Describe("", func() {
}
ginkgo.By("Waiting for sleep pods to be running")
- err = kClient.WaitForJobPodsRunning(dev, job1.Name, parallelism, 30*time.Second)
+ err = kClient.WaitForJobPodsRunning(dev, job1.Name, parallelism, 60*time.Second)
Ω(err).NotTo(gomega.HaveOccurred())
- err = kClient.WaitForJobPodsRunning(dev, job2.Name, parallelism, 30*time.Second)
+ err = kClient.WaitForJobPodsRunning(dev, job2.Name, parallelism, 60*time.Second)
+ Ω(err).NotTo(gomega.HaveOccurred())
+
+ ginkgo.By("Deleting sleep jobs")
+ err = kClient.DeleteJob(job1.Name, dev)
+ Ω(err).NotTo(gomega.HaveOccurred())
+ err = kClient.DeleteJob(job2.Name, dev)
Ω(err).NotTo(gomega.HaveOccurred())
ginkgo.By("Deleting sleep pods")
Review Comment:
I think we can resolve this with a later PR. For now, let's just get this merged. The code here is very explicit, so that we clean things up in a timely manner. Job deletion can be slow.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] craigcondit commented on pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
craigcondit commented on PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#issuecomment-1316136519
Rebased on master since we have conflicts.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] craigcondit commented on a diff in pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#discussion_r1023406758
##########
test/e2e/recovery_and_restart/recovery_and_restart_test.go:
##########
@@ -171,9 +171,15 @@ var _ = ginkgo.Describe("", func() {
}
ginkgo.By("Waiting for sleep pods to be running")
- err = kClient.WaitForJobPodsRunning(dev, job1.Name, parallelism, 30*time.Second)
+ err = kClient.WaitForJobPodsRunning(dev, job1.Name, parallelism, 60*time.Second)
Ω(err).NotTo(gomega.HaveOccurred())
- err = kClient.WaitForJobPodsRunning(dev, job2.Name, parallelism, 30*time.Second)
+ err = kClient.WaitForJobPodsRunning(dev, job2.Name, parallelism, 60*time.Second)
+ Ω(err).NotTo(gomega.HaveOccurred())
+
+ ginkgo.By("Deleting sleep jobs")
+ err = kClient.DeleteJob(job1.Name, dev)
+ Ω(err).NotTo(gomega.HaveOccurred())
+ err = kClient.DeleteJob(job2.Name, dev)
Ω(err).NotTo(gomega.HaveOccurred())
ginkgo.By("Deleting sleep pods")
Review Comment:
I think we can resolve this with a later PR. For now, let's just get this merged.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] codecov[bot] commented on pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#issuecomment-1258833116
# [Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/466?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#466](https://codecov.io/gh/apache/yunikorn-k8shim/pull/466?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b75bdbe) into [master](https://codecov.io/gh/apache/yunikorn-k8shim/commit/244a28f3175b5d4a1c314aa621a45cc9010bca4a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (244a28f) will **decrease** coverage by `0.22%`.
> The diff coverage is `25.67%`.
```diff
@@ Coverage Diff @@
## master #466 +/- ##
==========================================
- Coverage 67.00% 66.77% -0.23%
==========================================
Files 41 41
Lines 6767 6809 +42
==========================================
+ Hits 4534 4547 +13
- Misses 2062 2092 +30
+ Partials 171 170 -1
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-k8shim/pull/466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/cache/context.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `44.96% <0.00%> (ø)` | |
| [pkg/plugin/support/framework\_handle.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3BsdWdpbi9zdXBwb3J0L2ZyYW1ld29ya19oYW5kbGUuZ28=) | `22.03% <2.77%> (-25.97%)` | :arrow_down: |
| [pkg/plugin/predicates/predicate\_manager.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3BsdWdpbi9wcmVkaWNhdGVzL3ByZWRpY2F0ZV9tYW5hZ2VyLmdv) | `50.73% <45.71%> (+0.54%)` | :arrow_up: |
| [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `43.70% <100.00%> (+1.96%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] craigcondit commented on a diff in pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#discussion_r1019358523
##########
deployments/image/plugin/start-yunikorn-plugin.sh:
##########
@@ -17,13 +17,15 @@
# limitations under the License.
cd "${HOME}" || exit
+
+# Embed scheduler name into config file
+sed 's/schedulerName:.*/schedulerName: '"${SCHEDULER_NAME}"'/' < "${SCHEDULER_CONFIG}" > /tmp/scheduler-config.yaml || exit
Review Comment:
New PR uploaded with schedulername removed.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] craigcondit commented on pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
craigcondit commented on PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#issuecomment-1261122001
> Hi, wanted to mention that building against 1.23.12 fixed [YUNIKORN-1253](https://issues.apache.org/jira/browse/YUNIKORN-1253) Tested on openshift 4.10.15 (Which is using v1.23.5+9ce5071), with yunikorn Helm chart deployed as standalone scheduler w/o the admission controller. Guessing the problem was that Generic ephemeral volumes were added in 1.23
Unfortunately, there's still some issues here, especially when running in plugin mode. It seems the default scheduler logic in 1.23.x puts the pods into an internal unschedulable queue, where they remain for quite some time (sometimes over a minute). Until that's solved, this will be blocked.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#issuecomment-1261708174
> Unfortunately, there's still some issues here, especially when running in plugin mode. It seems the default scheduler logic in 1.23.x puts the pods into an internal unschedulable queue when they are not yet ready to be scheduled by YuniKorn, where they remain for quite some time (sometimes over a minute). Until that's solved, this will be blocked.
That should hopefully change when we have KEP-3521 fixed on the K8s side. With that change in place a pod can be marked not ready for scheduling and YuniKorn can release 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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] craigcondit commented on a diff in pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#discussion_r1019348286
##########
deployments/image/plugin/start-yunikorn-plugin.sh:
##########
@@ -17,13 +17,15 @@
# limitations under the License.
cd "${HOME}" || exit
+
+# Embed scheduler name into config file
+sed 's/schedulerName:.*/schedulerName: '"${SCHEDULER_NAME}"'/' < "${SCHEDULER_CONFIG}" > /tmp/scheduler-config.yaml || exit
Review Comment:
The point was to allow customizing the scheduler name, which as of 1.23 has to be in the scheduler-config file. However, as part of the configv2 redesign, we're removing the ability to change the scheduler name (which never really worked anyway). So I'm removing this code.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] craigcondit commented on pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
craigcondit commented on PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#issuecomment-1276742098
Code coverage decrease is expected, as there are now several no-op stub functions necessary in `framework_handle.go`. These are never called but are needed to satisfy the `FrameworkHandle` interface in K8s 1.23.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] VladZ97 commented on pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
VladZ97 commented on PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#issuecomment-1261106975
Hi, wanted to mention that building against 1.23.12 fixed [YUNIKORN-1253](https://issues.apache.org/jira/browse/YUNIKORN-1253)
Tested on openshift 4.10.15 (Which is using v1.23.5+9ce5071), with yunikorn Helm chard deployed as standalone scheduler w/o the admission controller.
Guessing the problem was that Generic ephemeral volumes were added in 1.23
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] pbacsko commented on a diff in pull request #466: [YUNIKORN-1196] Build against Kubernetes 1.23.12
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #466:
URL: https://github.com/apache/yunikorn-k8shim/pull/466#discussion_r1022910579
##########
test/e2e/recovery_and_restart/recovery_and_restart_test.go:
##########
@@ -171,9 +171,15 @@ var _ = ginkgo.Describe("", func() {
}
ginkgo.By("Waiting for sleep pods to be running")
- err = kClient.WaitForJobPodsRunning(dev, job1.Name, parallelism, 30*time.Second)
+ err = kClient.WaitForJobPodsRunning(dev, job1.Name, parallelism, 60*time.Second)
Ω(err).NotTo(gomega.HaveOccurred())
- err = kClient.WaitForJobPodsRunning(dev, job2.Name, parallelism, 30*time.Second)
+ err = kClient.WaitForJobPodsRunning(dev, job2.Name, parallelism, 60*time.Second)
+ Ω(err).NotTo(gomega.HaveOccurred())
+
+ ginkgo.By("Deleting sleep jobs")
+ err = kClient.DeleteJob(job1.Name, dev)
+ Ω(err).NotTo(gomega.HaveOccurred())
+ err = kClient.DeleteJob(job2.Name, dev)
Ω(err).NotTo(gomega.HaveOccurred())
ginkgo.By("Deleting sleep pods")
Review Comment:
I think this part can be removed. This is exactly why I filed YUNIKORN-1404. No need to individually delete pods (optionally, we can wait for them to disappear).
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org