-
Notifications
You must be signed in to change notification settings - Fork 169
Add recreate-vms-created-before option to recreate and deploy commands #710
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
base: main
Are you sure you want to change the base?
Conversation
CLI counterpart to !2656 The goal is to allow resumption of failed bosh repave (recreate) from where it failed, speeding up repave operations.
|
See other pr for manual testing |
Replace opaque string types with proper time.Time for the recreate-vms-created-before and vms-created-before flags. This provides type safety, validation at parse time, and clearer error messages for invalid RFC 3339 timestamps. - Add TimeArg type with UnmarshalFlag for parsing RFC 3339 timestamps - Update DeployOpts and RecreateOpts to use TimeArg - Update director UpdateOpts and RecreateOpts to use time.Time - Add comprehensive tests for TimeArg parsing and formatting
df07972 to
8cd5366
Compare
| SkipUploadReleases bool `long:"skip-upload-releases" description:"Skips the upload procedure for releases"` | ||
| Recreate bool `long:"recreate" description:"Recreate all VMs in deployment"` | ||
| RecreatePersistentDisks bool `long:"recreate-persistent-disks" description:"Recreate all persistent disks in deployment"` | ||
| RecreateVMsCreatedBefore TimeArg `long:"recreate-vms-created-before" description:"Only recreate VMs created before the given RFC 3339 timestamp (requires --recreate)"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What time zone are we enforcing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe RFC3339 requires a timezone be specified in the timezone string: https://datatracker.ietf.org/doc/html/rfc3339#section-5.6 and UTC is the preferred format. I dont feel strongly, but i was inclined to just allow what ever timezones the spec allows (Z +/- an offset from UTC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ill add tests to make sure it behaves the way we expect; based on yours an arams feedback.
- make sure it handles dates without a +00:00
- make sure all timestamps are UTC timezone internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a timestamp-based filter to recreate and deploy --recreate so operators can resume a failed repave by only recreating VMs created before a given point in time.
Changes:
- Extend director
RecreateOpts/UpdateOptsto carry aVMsCreatedBefore/RecreateVMsCreatedBeforetimestamp and send it asrecreate_vm_created_beforein requests. - Add CLI flags for
deploy(--recreate-vms-created-before) andrecreate(--vms-created-before) and wire them through to director calls. - Introduce
TimeArgflag type with parsing + tests, and add request/CLI tests covering the new query parameter.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| director/interfaces.go | Adds timestamp fields to recreate/update option structs. |
| director/deployment.go | Plumbs timestamp into ChangeJobState/UpdateDeployment query params. |
| director/deployment_test.go | Adds tests asserting the new query param is sent. |
| cmd/recreate.go | Wires new recreate timestamp option into director recreate opts. |
| cmd/recreate_test.go | Tests that recreate passes the timestamp through. |
| cmd/deploy.go | Wires new deploy recreate timestamp option into director update opts. |
| cmd/deploy_test.go | Tests that deploy passes the timestamp through. |
| cmd/opts/time_arg.go | New flag type for parsing RFC3339(-like) timestamps. |
| cmd/opts/time_arg_test.go | Unit tests for TimeArg parsing/formatting behavior. |
| cmd/opts/opts.go | Adds CLI flags for the new timestamp filters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| updateOpts := boshdir.UpdateOpts{ | ||
| RecreatePersistentDisks: opts.RecreatePersistentDisks, | ||
| Recreate: opts.Recreate, | ||
| Fix: opts.Fix, | ||
| SkipDrain: opts.SkipDrain, | ||
| DryRun: opts.DryRun, | ||
| Canaries: opts.Canaries, | ||
| MaxInFlight: opts.MaxInFlight, | ||
| Diff: deploymentDiff, | ||
| ForceLatestVariables: opts.ForceLatestVariables, | ||
| RecreatePersistentDisks: opts.RecreatePersistentDisks, | ||
| Recreate: opts.Recreate, | ||
| RecreateVMsCreatedBefore: opts.RecreateVMsCreatedBefore.Time, | ||
| Fix: opts.Fix, |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeployCmd always passes RecreateVMsCreatedBefore through to the director when the flag is set, but there’s no validation that --recreate was also provided (even though the flag description says it’s required). Add a check to error (or automatically imply Recreate=true) when a timestamp is provided without opts.Recreate, otherwise the director may reject/ignore the parameter.
| Canaries: opts.Canaries, | ||
| MaxInFlight: opts.MaxInFlight, | ||
| Converge: true, | ||
| VMsCreatedBefore: opts.VMsCreatedBefore.Time, | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--vms-created-before is only applied in the converge branch. If a user runs recreate with --no-converge and provides this flag, it will be silently ignored. Consider validating and returning an error when VMsCreatedBefore is set together with --no-converge (or otherwise document/handle the behavior explicitly).
| t, err := time.Parse(time.RFC3339, data) | ||
| if err != nil { | ||
| // Try RFC3339 without timezone suffix, assume UTC | ||
| // Format: "2006-01-02T15:04:05" | ||
| t, err = time.Parse("2006-01-02T15:04:05", data) |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UnmarshalFlag uses time.Parse(time.RFC3339, ...), which rejects valid RFC 3339 timestamps that include fractional seconds (e.g. 2026-01-01T00:00:00.123Z). If the flag is documented as “RFC 3339”, consider trying time.RFC3339Nano (or accepting both) so users can paste timestamps produced by common tooling.
| func (a TimeArg) AsString() string { | ||
| if a.IsSet() { | ||
| // Always output in UTC with Z suffix for consistency | ||
| return a.Format(time.RFC3339) |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsString’s comment says it always outputs UTC with a Z suffix, but it calls a.Format(time.RFC3339) directly. If TimeArg is constructed programmatically with a non-UTC location, this will output an offset instead of Z. Consider formatting a.UTC() (or enforcing UTC on assignment) to match the documented behavior.
| return a.Format(time.RFC3339) | |
| return a.UTC().Format(time.RFC3339) |
CLI counterpart to cloudfoundry/bosh#2656
The goal is to allow resumption of failed bosh repave (recreate) from where it failed, speeding up repave operations.