openconfig
gnoi/os: update comment for package_size to note the field is recommended
If the package_size field is unspecified, then the target will proceed without knowing how large the incoming image is. This can lead to the target returning an error mid-RPC if it runs out of disk space to store the incoming image.
To handle this the target is left with two options. Either it must:
- assume some default size of the image
- not free-up any space for the incoming image
If the target does 1 then the assumed image size may be less than the actual image (which would cause the target to not free up enough space) OR the assumed image size may be larger than the actual image (which may cause the target to free up excess space, possibly deleting images needlessly).
If the target does 2 then the RPC will fail if there is not enough space, irrespective of if there were unused images, and someone would need to manually clean up the unused images from the target before running the RPC again.
In both cases, we could still fail the RPC if we don't have enough space.
We should therefore make this package_size field required, so that:
- the target can fail gracefully if the incoming package is too large rather than mid-way through the RPC.
- the target can free-up space properly
Pull Request Test Coverage Report for Build 17646925938
Details
- 0 of 0 changed or added relevant lines in 0 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage remained the same at 0.0%
| Totals | |
|---|---|
| Change from base Build 17599230718: | 0.0% |
| Covered Lines: | 0 |
| Relevant Lines: | 12442 |
💛 - Coveralls
This can lead to the target returning an error mid-RPC if it runs out of disk space to store the incoming image.
I don't think that's a problem? RPCs can fail at any time, that's normal.
I'd vote to keep this as is (optional); operators can choice if they want to use it.
(elaborating on my points in the commit message)
the driving rationale behind learning requiring this var in advance is that it allows the server to perform cleanup to free up space for the incoming image. The proto specifies that: https://github.com/openconfig/gnoi/blob/0ed775fe0c4dc29a171b6547791a5c8646196661/os/os.proto#L55-L59
Its not possible for the target to reliably know how much space that it needs to free up if the package size is not known in advance. Any guesses the target makes about the size of the incoming image may result in either: a). not enough space being cleared out, in which case the RPC will fail b). too much space being cleared out, in which case we may delete other OS images that we could've preserved.
(critically the target can hit either of these conditions, depending on the size of the image being transferred and whether it is larger or smaller than we have guessed it would be).
If it is the case that we end up doing (a), then the network operator must manually access the system and clean up space (and imo we shouldnt ever have a stage in an RPC-based workflow where the network operator manually intervenes with the system like this, because it defeats the point of using an RPC) ...or they must determine the package size and then add that into the TransferRequest and then restart the RPC, so the target can know how much space it needs to clean.
But if they have the capability to add the package size into the TransferRequest (which they should have since they store the image somewhere) and restart the RPC, then they should be just doing that initially, and therefore remove the chance of ever even hitting an issue related to the package size
It appears you're making an implicit assumption that image management depends on receiving an RPC call.
That's not the only option and it doesn't have to be this way. Even the quoted part says and always proactively frees up space for incoming new OS packages. (I'd argue that doing so after receiving a request implies a reactive behavior, instead of proactive)
and therefore remove the chance of ever even hitting an issue related to the package size
reduce, probably (if you implement this logic and don't have another mechanism that clears old/unused images). I don't think you can eliminate this possibility (e.g. a worst case scenario: there's only one image left and still not enough space for a new image. Unless you start deleting something else..)
I don't think making this mandatory is justified. And this is a non-backward compatible change.
I'd rather suggest expanding description/wording about the use of package_size, something along the lines of it is recommended to specify the size ... and implementations may rely on it to clean up the disk space if they choose to do so
yeah i think the key point is that this would be backward-incompatible. I've updated the description as suggested
This service is built on the premisse that the OS handles it's own file system according to the expectations defined in this proto. If an implementer chooses to implement this interface without following this premisse, then the same external system that is used to manage the file system should be used to quantify it's available space.
