Refactor and enhance SGCP drivers and caching mechanisms - #1103
Open
cgalibern wants to merge 34 commits into
Open
Refactor and enhance SGCP drivers and caching mechanisms#1103cgalibern wants to merge 34 commits into
cgalibern wants to merge 34 commits into
Conversation
This was referenced Sep 3, 2026
- Cache getAliases responses with a configurable TTL (default 30s).
- Use a sentinel value ("null") to cache 404 (no aliases) without
poisoning the cache with nil.
- Invalidate the cache after every mutation (create, update, delete).
- Explicitly clear the cache before Start, Stop, and Status to ensure
fresh data.
- Expose CacheTTL on the mgr struct so tests can disable the cache
(set to 0) and avoid filesystem permission issues.
Changes:
- drivers/resipsgcp_dnsalias/main.go
- drivers/resipsgcp_dnsalias/mgr.go
- drivers/resipsgcp_dnsalias/main_test.go
- Stop clearing alias cache before every read; only invalidate after mutations (create, update, delete) to allow cache reuse and reduce API traffic. - Include endpoint and secret (SHA-256 hashed) in cache signature to isolate cache entries between different SGCP authentication contexts.
- Extend util/sgcpserverfortest/main.go with handlers for the new SGCP DNS CNAME records API (list, create, get, update/patch, delete) and a basic CG endpoint. - Add util/sgcpserverfortest/users.yaml for test credentials.
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.
- Handle 412 status before transport error in Switchover to allow failover fallback - Use env.HasDaemonOrigin() for daemon origin check, keep legacy "daemon" compatibility - Remove unbounded consistency group cache in Status to avoid stale state - Add Resync method to wire sync-resume through the resource framework - Verify expected availability zone after start operation completes - Guard against empty georedundancy targets and evaluate all geo targets in checkResumable - Reject failed/rollback states in mixed replication/georedundancy resume checks
The az keyword defaults to {node.labels.az}, and an unset node label
evaluates to an empty string instead of failing. The resource used to
configure fine with an empty az, and the start action then asked the
provider for a switchover to the "" availability zone.
Validate az in Configure, where the secret and the endpoint already are,
and say in the keyword doc that a node without the az label has to set
the keyword.
The driver package was never imported, so its init() never ran and driver.Register was never called: fs#1.type=sgcp_nfs_cg was reported as an unknown driver, and the capability scanner never announced drv.fs.sgcp_nfs_cg.
The text had the two operations swapped: the start action calls failover when switchover fails with status code 412, not the other way round. Say which conditions actually enable that fallback, and what --force does.
The tests installed the sgcp configuration in a tempdir, but left the agent root alone, so the ageing caches the drivers keep and the locks they take went to the node /var/lib/opensvc: a run left 62 cache entries behind, and needed the privileges to write them. Make the tempdir the agent root for the duration of the test, and move the disabled flag there too. That flag is an absolute path, stat'ed on every action, and file.Exists() reports a permission error as an existing file: an unprivileged run used to see the sgcp support as disabled, and the resource start silently did nothing.
IsDisabled() went through file.Exists(), which reports every stat error but "does not exist" as an existing file. A permission or io error on the disabled flag path thus disabled the sgcp support: the consistency group start returned success without switching anything over, and the instance was declared up in the wrong availability zone. Stat the flag here instead, and tell the three outcomes apart. The callers now act on the undecidable one: the actions that have work to skip report it, the status evaluations log it and stay n/a, and the consistency group stop, which has nothing to undo, only warns. Two more things fall out of the rewrite. The configuration is read through GetConfig(), under the lock that guards its writes and without dereferencing a nil config. And the dir argument, ignored so far, is now what a relative disabled_flag path is resolved against, instead of the current directory of whoever runs the command.
The two fs drivers gate their start, stop and status on the disabled flag, this one never did. Creating the flag stopped the filesystem and consistency group work on a node, and left the alias driver creating, retargeting and deleting records through the api. Gate the three actions the same way, and cover it: no api call at all when the flag is set, the actions run when it is not, and an undecidable flag is reported rather than taken for a disable.
createOrUpdate() cleared the cache after replacing m.alias with what the api returned. The signature hashes the alias identity, so a resource configured without the uuid keyword cached its aliases under the empty uuid, then dropped the entry of the completed one, which nothing had ever written. The stale aliases stayed readable for the whole ttl: a status evaluated right after a successful start reported the target the alias had before. Capture the signature before the read that fills it, and drop that one.
The fallback also accepted a bare OSVC_ACTION_ORIGIN=daemon, a value no daemon ever sets: imon, nmon, the api and the scheduler set daemon/monitor, daemon/api and daemon/scheduler, which env.HasDaemonOrigin() already matches. The only way to have that value was to export it by hand, which is a way around a gate that exists to tell an orchestration apart from an operator start, where the operator asks for a failover with --force. The literal was there for the test. Have it set daemon/monitor instead, and the test refusing the fallback set the user origin.
cache.enabled was read nowhere but in a test asserting it parses, and the dns alias driver aged its entries on a 30s constant of its own while the consistency group driver read cache.ttl_seconds. Two drivers, two policies, and a setting that did nothing. Drop cache.enabled, and have the alias driver take its ttl from the configuration like the other one. A zero ttl_seconds now disables the caching, which is what the removed flag was meant to do. A deployed configuration still naming enabled keeps loading: the yaml decoding ignores what the type does not declare.
syncResume() logged the reason checkResumable() gave, under a leftover debug prefix, and returned a bare "still not resumed". The operator running sync resync got the verdict without the explanation. Wrap the reason in the returned error. A checkResumable() that accepts the group again keeps the plain message: nothing went wrong there, the resume just did not take.
GetCg() reads past the cache, and threw away what Clear() had to say. A clear that fails leaves the read behind it serving the entry the caller asked to go without, and the wait loops poll that same status every two seconds until they time out. Return it instead. A cold cache is not an error: fcache Clear() returns nil for an entry that is not there. Also drop cacheClear(), which nothing called.
The method took an az and discarded it right away. The resume payload carries no operationParameters: unlike the switchover and the failover, the operation resumes the group toward the targets it is already configured with. Say so, and stop asking the caller for a zone.
The wait loops logged their message with Warnf(msg), and the status loggers were called with the result of an inner Sprintf. Both hand a computed string over where a format is expected, and every one of these messages interpolates text the provider chose: a status or an availability zone carrying a percent sign came out mangled. Pass the arguments instead. go vet says nothing here, its printf analysis not knowing these loggers.
Stop moves nothing: the group stays in the availability zone the last start switched it to, and the start of the instance elsewhere is what moves it. Passing silently left an operator watching a stop wondering whether the action did anything, and a reader wondering whether the empty body was a hole. Log the three states the action can be in, and write down why it has nothing to undo.
TestStatus asserts six evaluations share one cache entry, and the test driver aged its entries after a second: the assertion held only as long as the six calls fit in that second. A 1.1s pause where a loaded runner would stall is enough to fail it. Give the test driver a ttl no run can outlive. The test that wants a fresh read already drops the entry, which is what the driver does. Also loop over the cached evaluations, asserting each, instead of assigning a variable five times and reading none of them.
The resume branch applied its outcome before asking PatchResumeFunc, so a test injecting a failure still got a resumed group, and could not tell what the driver does with a provider that refuses. Ask the hook first and return on its error. The order was not arbitrary: Update() replaces the map entry, leaving the handler pointer stale, so mutating after the hook would silently discard what the hook wrote. The hook now replaces the outcome instead of gating it, which is what the three tests using it already expressed. Write that contract down, the switchover and failover hooks being failure injectors instead.
loadUsers() logged every client secret, then printed the whole map with %#v, which printed them again. The users.yaml committed here holds made up credentials, but nothing stops an operator from pointing this at an account of their own. Log the user and client ids and the scopes, which is what tells the file was read. Also have the consistency group stub answer "ready" instead of "online", a state the driver knows: it used to warn about the status it got, and poll it until the action timed out.
Callers that want to tell the scheduler apart from the rest of the daemon had to compare Origin() themselves. Give the scheduler the predicate its two siblings already have. The doc comments of those siblings named values the daemon never sets, "daemon" for HasDaemonOrigin and "daemon/imon" for the monitor one. Name the constants instead, and cover the three predicates with a table that pins the bare "daemon" and "scheduler" strings as origins of no one.
A status asked for by anyone else was answered from the ageing cache the scheduler fills, so an operator could be shown a consistency group in the availability zone it left hours ago. The caches exist to keep the scheduler's repeated evaluations off the provider api, not to answer the question an operator is asking. Drop the entry at the top of the three Status(), where the drivers already have the invalidation their actions use, under a sgcphelper.NeedsCacheClear() the three share. That rule lives with the drivers rather than in util/sgcp, which reads no core package.
Configure() read the endpoint from the keyword, fell back to the configuration and refused an empty value, then never applied it. Every request kept going to dns.base_url, and the only thing the keyword reached was the cache signature. WithDNSBaseURL() has been there since the driver was written, called from nowhere. The two fs drivers call their WithFileURL() equivalent in the same place. Covered end to end, because a mocked api provider builds no url and is how the bug survived until now: the alias is served by a test server the endpoint keyword names, while the configuration points at a port nothing listens on. That needs the real api, hence the credentials seam the resfssgcp_nfs driver already has.
The api reports every status over 400 as an error, and getFileInfo() tested the error first, so the not-found branch below it never ran, nor the four "fileInfo == nil" guards it fed. The branch would not have worked either: it returned no bytes, which the cache stored and the unmarshal then choked on. Read the status code first, cache the absence as a null document the way the dns alias driver does, and turn it back into no filesystem at all on the way out. Same order in deleteNFSClient(), whose "consistency group is not in ready" was unreachable for the same reason: a refused drop showed up as a bare HTTP 412. The tests drive the real api against a test server, a fake being unable to reproduce what this is about. Drop the unreachable switches, and a private copy of CheckStatusCode() that no one called.
| t.StatusLog().Info("xaas status disabled") | ||
| return status.NotApplicable | ||
| } | ||
| if sgcphelper.NeedsCacheClear() { |
Contributor
There was a problem hiding this comment.
Should we handle the cache clear error here instead of only logging it?
If the cache cannot be cleared, GetCachedCg() may return stale CG information. GetCg() already treats a cache clear failure as an error for this reason, to avoid polling on stale data until it times out.
Contributor
Author
There was a problem hiding this comment.
👍 you are right, we should stop here
TODO: create a new type Convertor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
Refactor:
ageingcachefor improved consistency group cache handling.ErrPreconditionfor better clarity and control.Fixes:
resfssgcp_nfs_cgdriver:Features:
resfssgcp_nfs_cgresource driver to manage NFS consistency groups with capabilities like Switchover, Sync-resume, and enhanced status reporting.