Upgrade agent sandbox v0.5.3 - #446
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
+ Coverage 58.41% 59.87% +1.45%
==========================================
Files 36 36
Lines 3463 3621 +158
==========================================
+ Hits 2023 2168 +145
- Misses 1231 1234 +3
- Partials 209 219 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8294a36 to
822dc7b
Compare
9928ed7 to
83002f1
Compare
|
#448 has merged into /retest |
|
@ranxi2001: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
doing it.. |
36a4332 to
dea727d
Compare
dea727d to
d769fc5
Compare
ebc7670 to
38380c8
Compare
aedd7bb to
2eefda6
Compare
3c5dcfa to
a0f4882
Compare
ranxi2001
left a comment
There was a problem hiding this comment.
Reviewed exact head a0f4882. The Kubernetes/code-generator version skew is fixed and the current checks pass. I found one authorization regression and three current-head compatibility/operational gaps in the inline comments.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Signed-off-by: Safiya <147792763+safiya2610@users.noreply.github.com>
0f57d46 to
a524029
Compare
Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
ranxi2001
left a comment
There was a problem hiding this comment.
Thanks for the updates. I verified the migrated-claim Ready wait, Store owner propagation, and codegen tool-path fixes on 353f1df. I left one remaining upgrade-path comment below.
| done | ||
|
|
||
| echo "Running migration migrate phase..." | ||
| /tmp/migrate.sh --phase=migrate |
There was a problem hiding this comment.
[P2] Stop before migration when webhook readiness times out
After the 30th failed probe, this branch prints diagnostics but does not exit. The loop ends and /tmp/migrate.sh --phase=migrate still runs, so the mutating storage migration can start even though the conversion webhook never became reachable.
Could you exit non-zero after the timeout diagnostics so migration only starts after a successful readiness probe?
Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
ranxi2001
left a comment
There was a problem hiding this comment.
Re-reviewed exact head d7333cc. The webhook timeout path is now fail-closed, so that thread is fixed. I am not adding duplicate inline threads, but three previously raised PR-scope items still remain in the current tree:
- The migrated
upgrade-bound-claimis checked for Ready and identity preservation, but the test still never deletes that same claim, verifies the captured Sandbox/Pod UIDs are garbage-collected, or proves the same pool refills. The later warm-pool test creates a fresh v1beta1 lineage, so it does not close #438's migrated-object lifecycle requirement. Existing lifecycle thread. - The dependency bump still makes an existing AgentRuntime
workloadRefunrepresentable in the embedded typedPodSpec; the current test only constructs a fresh v0.36 PodSpec and checks an image. The root guide notes the transition, but the mirrored guide and an old-object compatibility test are missing, and no maintainer has accepted dropping this stored scheduling intent. Existing compatibility thread. - Both mandatory migration guides still wait for the conversion webhook with an unbounded
untilloop. A permanent failure hangs forever without diagnostics or a non-zero exit. Existing docs thread.
No new current-head P1/P2 findings beyond these. I would hold /lgtm until they are addressed or explicitly accepted by a maintainer.
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
docs: fix webhook wait loops and add workloadRef transition note test: add old-object compatibility test for AgentRuntime Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
…atibility Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
ranxi2001
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 86e45ab. The bounded webhook probes in both guides now close that item. The new E2E block also runs in both current jobs, but two compatibility gaps remain:
-
[P1] The refill assertion crosses object lineages. The job log shows bootstrap classifies
upgrade-bound-claimas warm-started and saysno shadow needed, then createsshadow-pool-e2e-upgrade-templatefor the separate cold claim with desired replicas 0. Deleting the standalone bound claim and scaling that unrelated pool from 0 to 1 proves initial fill, not refill of the migrated claim's source pool. The GC loop also treats anykubectl geterror as deletion and never verifies the captured Pod UID or a replacement Sandbox UID. Please seed a real v0.4.6 warm pool, adopt from it, then delete the same migrated claim and require the original Sandbox/Pod UIDs to disappear and that same pool to return to desired capacity with a different Sandbox UID. -
[P2] The old-object test does not model a valid old object or verify its scheduling contract. The previous CRD required both
workloadRef.nameandworkloadRef.podGroup, but the fixture supplies onlyname; the assertions then prove only that an unrelated container image survives whileworkloadRefis silently dropped. BecauseworkloadRef(name,podGroup,podGroupReplicaKey) is not shape-equivalent toschedulingGroup(podGroupName), "transition those fields" does not define the migration. If dropping this alpha field is intentional, please use a schema-valid legacy payload and explicitly test and document which scheduling semantics are unsupported; otherwise map the supported intent.
I would still hold /lgtm until these two gaps are closed or explicitly accepted by a maintainer.
…ue UID docs: detail workloadRef vs schedulingGroup compatibility boundary test: use schema-valid legacy workloadRef payload in old-object test Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
Signed-off-by: safiya2610 <lci2023053@iiitl.ac.in>
ranxi2001
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 8a528f1. The schema-valid legacy workloadRef fixture and the explicit unsupported compatibility boundary now close that item. One lifecycle gap remains:
- [P1] The refill check still observes a different object lineage.
e2e-upgrade-warmpoolowns its own ready Sandbox, whileupgrade-bound-sandboxis created separately and manually attached toupgrade-bound-claimwithout that pool's ownership or labels. The v0.5.3 conversion therefore deriveswarmPoolRef.name=upgrade-boundfrom the bound Sandbox name, note2e-upgrade-warmpool. After deleting the claim,readyReplicascan already be 1 and the generic warm-pool label query can return the pool's pre-existing member, whose UID necessarily differs from the unrelated bound Sandbox UID. The test can therefore pass without the deletion causing a refill. Please let the v0.4.6 pool create and hand out the actual member, capture that pool/Sandbox/Pod lineage, then delete the same migrated claim and require the original UIDs to disappear and the same pool to produce a replacement Sandbox UID.
Description
This PR upgrades the
sigs.k8s.io/agent-sandboxdependency fromv0.4.6to the latest stable releasev0.5.3.As part of this upgrade, all internal workload manager logic and tests have been migrated to use the newly introduced
agents.x-k8s.io/v1beta1andextensions.agents.x-k8s.io/v1beta1APIs, replacing the olderv1alpha1types.Key Changes
sigs.k8s.io/agent-sandboxtov0.5.3ingo.modandgo.sum.Sandbox,SandboxClaim,SandboxWarmPool, andSandboxTemplateusages inpkg/workloadmanagertov1beta1.SandboxClaimconstruction to useWarmPoolRefinstead of the deprecatedTemplateRef.TestSandboxVolumeClaimTemplatesImmutabilityto explicitly enforce and test the new immutability behavior ofSandbox.spec.volumeClaimTemplates.test/e2e/e2e_test.goand all internalpkg/workloadmanagertests to construct and validatev1beta1objects. Fixed namespace validation defaulting logic in the HTTP creation handler.run_e2e.shto fetch thev0.5.3sandbox-with-extensions.yamlmanifest.Fixes #438