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
CLI counterpart to !2656 The goal is to allow resumption of failed bosh repave (recreate) from where it failed, speeding up repave operations.
cf-foundation-community-automation
bot
moved this to Inbox
in Foundational Infrastructure Working Group
|
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
Alphasite
force-pushed
the
recreate-vms-timestamp
branch
from
df07972 to
8cd5366
Compare
github-project-automation
bot
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
| 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) |
beyhan
moved this from Pending Merge | Prioritized
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
* make sure timestamps are always UTC when rendering * allow --no-converge and --recreate-vms-created-before to be used together
4 participants
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.