Add recreate-vms-created-before option to recreate and deploy commands#710
Add recreate-vms-created-before option to recreate and deploy commands#710
Conversation
|
See other pr for manual testing |
df07972 to
8cd5366
Compare
cmd/opts/opts.go
Outdated
| 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.
What time zone are we enforcing here?
There was a problem hiding this comment.
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.
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.
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.
| Canaries: opts.Canaries, | ||
| MaxInFlight: opts.MaxInFlight, | ||
| Converge: true, | ||
| VMsCreatedBefore: opts.VMsCreatedBefore.Time, | ||
| } |
There was a problem hiding this comment.
--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).
cmd/opts/time_arg.go
Outdated
| func (a TimeArg) AsString() string { | ||
| if a.IsSet() { | ||
| // Always output in UTC with Z suffix for consistency | ||
| return a.Format(time.RFC3339) |
There was a problem hiding this comment.
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 !2656 The goal is to allow resumption of failed bosh repave (recreate) from where it failed, speeding up repave operations.
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
* make sure timestamps are always UTC when rendering * allow --no-converge and --recreate-vms-created-before to be used together
makes this feature a little nicer to use Signed-off-by: Nishad Mathur <nishad.mathur@broadcom.com>
41149c3 to
26f2273
Compare
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.