feat(drivers): implement SGCP NFS consistency group resource - #1067
feat(drivers): implement SGCP NFS consistency group resource#1067PaulJouvanceau wants to merge 1 commit into
Conversation
9ec69a7 to
f08a3d5
Compare
Add a new `resfssgcp_nfs_cg` resource driver to manage Scaleway NFS consistency groups (CG). This driver provides: - Switchover / failover of a CG to a target availability zone (AZ). - Sync-resume to re-establish replication after an incident. - Detailed status reporting including geo-redundancy and replication states. **Changes in `util/sgcp/file.go`:** - Add `GetConsistencyGroup` and `PatchConsistencyGroup` methods to the `FilesAPI` struct. These allow retrieving CG details and applying operations (switchover, failover, resume-replication) via the Scaleway API. **Implementation of `drivers/resfssgcp_nfs_cg/main.go`:** - Define data models (`CgInfo`, `GeoRedundancyInfo`, `ReplicationInfo`, etc.) that mirror the API response. - Implement `cgMgr` to abstract API calls and handle retries. - Implement `T` resource with configuration keywords: `uuid`, `az`, `secret`, `endpoint`, `timeout`, `failover`. - Add `Start()`: performs switchover (or failover when `--force` is set), with fallback to failover on precondition failure (412) if conditions are met. - Add `SyncResume()`: checks resumability via `checkResumable()` and calls the resume API, waiting for the CG to become ready. - Add `Status()`: displays CG status, geo-redundancy targets, and replication targets with appropriate log levels. - Implement caching (`cgInfoCache`) to reduce API calls during status polling. **Testing (`drivers/resfssgcp_nfs_cg/main_test.go`):** - Unit tests for helpers, `checkResumable`, `localRepStatus`, and `waitForFn` using JSON fixtures. - Integration tests covering `Start` (success, 412 fallback, force, already up, operation in progress) and `SyncResume` (success, already resumed, in progress) using a mocked API. - Add `util/sgcpcgtesthelper` package: an in-memory mock implementation of `cgAPI` with call counters and customisable callbacks (`PatchSwitchoverFunc`, `PatchFailoverFunc`, `PatchResumeFunc`). This allows precise control over API responses and state transitions.
f08a3d5 to
4f10ce7
Compare
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
🟡 Medium - Switchover 412 responses never reach the configured failover fallback
sgcp.Api.do returns a non-nil error for every HTTP status >= 400, including 412, but Switchover returns immediately on err before it examines code and wraps a 412 as PreConditionError. Consequently, a real API precondition failure cannot enter the daemon-only failover path in start, so resources configured with failover=true remain stopped on the exact condition this option is meant to recover from.
Show fix
Handle the HTTP status before returning the transport/API error, or otherwise preserve the 412 classification (wrapping the API error as PreConditionError) so start can safely apply its existing failover policy.
More info - Reply on this comment to give feedback or ignore the issue.
| return err | ||
| } | ||
| } | ||
| return t.waitStatus(ctx, []string{"ready"}) |
There was a problem hiding this comment.
🟡 Medium - Start treats any ready state as success without verifying the requested AZ
After an accepted switchover or failover, Start waits only for the consistency-group status to become ready; it never verifies that the returned availabilityZone is t.AZ. If the asynchronous operation completes in the old or another AZ because of a concurrent change or backend outcome, the resource returns success while this node still does not own the group, allowing dependents to proceed against the wrong replica.
Show fix
Make the post-operation poll require both the expected terminal status and cg.AvailabilityZone == t.AZ (or perform a final GetCg and fail if the ownership invariant is not met).
More info - Reply on this comment to give feedback or ignore the issue.
| geoStatus := cg.GeoRedundancies()[0].Status | ||
|
|
There was a problem hiding this comment.
🟡 Medium - Sync resume ignores additional georedundancy targets and can panic on an empty target list
The resumability checks classify georedundancy using GeoRedundancies()[0] even though the API model exposes a slice of target AZs, and hasGeoRedundancy can be true from Region alone when that slice is empty. A CG with the first target replicated but another target broken is incorrectly treated as already resumed, while a valid response with no target entries causes an index-out-of-range panic during sync resume.
Show fix
Validate that georedundancy has at least one target before indexing, and evaluate all target AZ statuses when deciding whether the group is already resumed or resumable rather than using only the first element.
More info - Reply on this comment to give feedback or ignore the issue.
| if os.Getenv("OSVC_ACTION_ORIGIN") != "daemon" { | ||
| t.Log().Errorf("%s, skip failover fallback, use --force if you want to try failover", msg) | ||
| return err |
There was a problem hiding this comment.
🟡 Medium - Daemon failover fallback checks an origin value the daemon never sets
The fallback compares OSVC_ACTION_ORIGIN to the literal daemon, but the daemon emits daemon/api, daemon/monitor, or daemon/scheduler, and the shared env.HasDaemonOrigin helper recognizes those values. Therefore the configured failover=true fallback is skipped for all real daemon-launched actions even after the 412 handling is corrected; the added test passes only because it injects the non-production value daemon.
Show fix
Use env.HasDaemonOrigin() (or compare against the three canonical daemon origin constants) instead of the literal environment string, and update the test to use a real daemon origin.
More info - Reply on this comment to give feedback or ignore the issue.
| func (t *T) SyncResume(ctx context.Context) error { | ||
| t.Log().Infof("sync resume ...") | ||
| t.clearGetCgCache() | ||
| defer t.clearGetCgCache() | ||
| if err := t.syncResume(ctx); err != nil { | ||
| t.Log().Errorf("sync resume failed") | ||
| return err | ||
| } | ||
| t.Log().Infof("sync resume succeed") | ||
| return nil |
There was a problem hiding this comment.
🟡 Medium - The new sync-resume implementation is not connected to the resource resync action
The resource framework dispatches sync-resync only to drivers implementing Resync(context.Context) error, but this driver adds SyncResume(context.Context) error instead. Since no caller of SyncResume exists in the repository, invoking the production sync-resync action sees no resyncer and returns ErrActionNotSupported; the new replication recovery path is therefore unreachable outside direct unit tests.
Show fix
Expose the implementation through the framework's expected Resync(context.Context) error method (or explicitly wire SyncResume into the resource action dispatcher) and test through resource.Resync/the object sync-resync action.
More info - Reply on this comment to give feedback or ignore the issue.
| if !contains([]string{"passive", "resuming"}, cg.Status) && | ||
| !contains([]string{"unknown", "replicated", "replicating"}, localRepStatus) { | ||
| return fmt.Errorf("sync resume not allowed on cg %s where status is %s and local replication status is %s", | ||
| t.UUID, cg.Status, localRepStatus) | ||
| } |
There was a problem hiding this comment.
🟡 Medium - Mixed replication/georedundancy resume permits failover and other unsafe states
For a mixed CG, checkResumableReplicationAndGeo allows any non-passive/non-resuming group whenever the local replication status is in its allowlist, including failover, failed, and rollback with local status unknown. syncResume then immediately issues resume-replication during that in-progress or failed operation, unlike the replication-only path which requires ready or resuming; this can create conflicting operations or leave the group in an unhealthy state.
Show fix
Reject failed, rollback, and unrelated operation states explicitly and align mixed-mode eligibility with the safe terminal/in-progress states supported by the API before sending resume-replication.
More info - Reply on this comment to give feedback or ignore the issue.
| func (t *T) getCgCached(ctx context.Context) (*CgInfo, error) { | ||
| if t.cgInfoCache != nil { | ||
| return t.cgInfoCache, nil | ||
| } | ||
| cg, err := t.mgr.GetCg(ctx) | ||
| if err == nil { | ||
| t.cgInfoCache = cg | ||
| } |
There was a problem hiding this comment.
🔵 Low - Consistency-group status is cached indefinitely across monitoring cycles
getCgCached stores the first successful API response, while Status never invalidates it; the cache is cleared only by local Start, Stop, or SyncResume actions. A remote failover or switchover therefore leaves every subsequent status poll reporting the old AZ and state until another local action occurs, making monitoring/status output stale precisely when external state changes are expected.
Show fix
Do not use an unbounded per-driver cache for Status, or add a bounded TTL/clear it at the end of each status evaluation while retaining any action-specific cache only for a single operation.
More info - Reply on this comment to give feedback or ignore the issue.
1 participant
Add a new
resfssgcp_nfs_cgresource driver to manage Scaleway NFS consistency groups (CG). This driver provides:Changes in
util/sgcp/file.go:GetConsistencyGroupandPatchConsistencyGroupmethods to theFilesAPIstruct. These allow retrieving CG details and applying operations (switchover, failover, resume-replication) via the Scaleway API.Implementation of
drivers/resfssgcp_nfs_cg/main.go:CgInfo,GeoRedundancyInfo,ReplicationInfo, etc.) that mirror the API response.cgMgrto abstract API calls and handle retries.Tresource with configuration keywords:uuid,az,secret,endpoint,timeout,failover.Start(): performs switchover (or failover when--forceis set), with fallback to failover on precondition failure (412) if conditions are met.SyncResume(): checks resumability viacheckResumable()and calls the resume API, waiting for the CG to become ready.Status(): displays CG status, geo-redundancy targets, and replication targets with appropriate log levels.cgInfoCache) to reduce API calls during status polling.Testing (
drivers/resfssgcp_nfs_cg/main_test.go):checkResumable,localRepStatus, andwaitForFnusing JSON fixtures.Start(success, 412 fallback, force, already up, operation in progress) andSyncResume(success, already resumed, in progress) using a mocked API.util/sgcpcgtesthelperpackage: an in-memory mock implementation ofcgAPIwith call counters and customisable callbacks (PatchSwitchoverFunc,PatchFailoverFunc,PatchResumeFunc). This allows precise control over API responses and state transitions.