Add takeover action for some device types

[pipex]

This is a proof of concept on how we could repurpose this module to indicate that a takeover is needed between certain versions for some device types.

Change-type: minor

[kb2ma]

The logic looks sound here, but it really jumps outside the logic framework in getHupActionType(). One alternative might be to define takeoverTargetVersion as a property of ActionConfig, which helps indicate it is a special case. Then below where those properties are looked up in getHupActionType(), first test if it's a takeover -- If targetTakeoverVersion is defined, currentVersion is less than tTV, and targetVersion is greater than tTV, then change the actionName to 'takeover'. Otherwise the action remains 'balenahup'.

We also could expand the value of takeoverTargetVersion to be an array if necessary.

[pipex]

I agree that is another way to do it, but I think that also jumps outside the logic no? You would still need to check in the else clause

balena-hup-action-utils/lib/index.ts

Lines 122 to 128 in f522a6b

	} else {
	// Takeover overrides the checks below for the device type
	if (this.isTakeoverRequired(deviceType, currentVersion, targetVersion)) {
	return 'takeover';
	}
	actionName = 'balenahup';
	}

for the targetTakeoverVersion which is pretty close to what we do with the isTakeoverRequired

Please correct me if I'm misunderstanding your point

[kb2ma]

If takeoverTargetVersion is a property of ActionConfig, we could consider takeover as a special case for balenahup -- and that is essentially its purpose. So in getHUPActionType(), the actionName initially would be 'balenahup'. Then takeoverTargetVersion would be read where the other actionConfig version values currently are read. Finally, at the end of getHUPActionType() there are some validation conditionals. Maybe just before those add the logic to see if takeoverTargetVersion is being crossed for balenahup, and if so return 'takeover' instead.

What I meant by jumping out of the flow is two things:

• The current logic in getHUPActionType() returns immediately if isTakeoverRequired() is true
• The definition of DeviceTypeConfig includes a special case for takeover minTargetVersion, which I found confusing

So the definition for jetson-xavier-nx-device-emmc could be like below, which also could be an array of strings for multiple jumps.

		'jetson-xavier-nx-devkit-emmc': {
			balenahup: {
				// NOTE: this version is here as a placeholder for
				// testing. Replace with the correct version before merging
				takeoverTargetVersion: '5.1.45+rev1',

I would say the key that makes this approach reasonable/possible is that getHUPActionType() returns only the name of the action.

[pipex]

Yes, I like this, it makes takeover kind of a special case of balenahup instead of its own process which is a good way to model it.

takeover minTargetVersion, which I found confusing

I agree this is confusing

I'll make the changes

[kb2ma]

@pipex, I was thinking about how to stage this work so it's easy to pull the trigger when all the other pieces are ready. How about this plan:

1. Add all 3 device types: jetson-xavier, jetson-xavier-nx-devkit, jetson-xavier-nx-devkit-emmc
2. Set the targetTakeoverVersion to 99.99.99 so it won't be invoked
3. Merge this PR

Then all we need to do is create a PR that sets the right version for those three DTs.

[kb2ma]

Why not just make 'takeover' another ActionName? As is, this likely forces clients to add this extra option as well. For example, see balena-proxy services.action-backend.actions.config.ts.

[kb2ma]

After more review I see how this decision is a judgement call. I'd still rather keep the names simple for clients and modify the type definition for the ActionsConfig as needed (just for action?). A comment for ActionName will help explain how it's different.

[pipex]

That was the first thing I did, but I would have to create an additional item here

balena-hup-action-utils/lib/config.ts

Line 20 in 8f8a770

actions: {

For those actions minSourceVersion and minTargetVersion are required and those values don't really make sense for takeover (or I don't know what to use)

I guess I could omit takeover in the type definition here

balena-hup-action-utils/lib/types.ts

Line 34 in 8f8a770

actions: { [K in ActionName]: ActionConfig };

but that also seems just as dirty?

I'm not entirely sure what is the best course of action

[kb2ma]

@pipex I created PR #37 that adds a commit with changes to add 'takeover' as an action name. It builds OK with balena-proxy. I didn't define an ActionConfig because that seemed simplest. But really we also could define one with empty string values for minSourceVersion and minTargetVersion.

I think it's perfectly fine to use comments in the implementation to explain the use of 'takeover'. We could modify the abstractions to accommodate how 'takeover' is used, but I don't see the value when we're not sure how our use of takeover will evolve.

As you say above, because of my implementation I modified the actions definition so an entry is optional. IMO, I would rather keep the interface to getHUPActionType() simple/unchanged. Let the implementation jump through whatever hoops are required to make it work.

I also added the expected minTakeoverVersion for the three device types.

[pipex]

@kb2ma I think the problem with doing it like that is that making the ActionConfig optional removes some guarantees that are used by the code in getHUPActionType().

The code below expects minSourceVersion and minTargetVersion to always be defined. The type in config.ts also enforces it and the code won't compile if you add an action without those values.

balena-hup-action-utils/lib/index.ts

Lines 143 to 159 in 8f8a770

	const {
	minSourceVersion,
	targetMajorVersion,
	minTargetVersion,
	minTakeoverVersion,
	maxTargetVersion,
	} = {
	...actionsConfig.actions[actionName],
	...defaultActions[actionName],
	...deviceActions[actionName],
	};
	if (bSemver.lt(currentVersion, minSourceVersion)) {
	throw new HUPActionError(
	`Current OS version must be >= ${minSourceVersion}`,
	);
	}

I think that if we want to use takeover as action we could set some nonsense values for those two variables (e.g v99.99.99) and add a comment about the reason for that configuration.

After that we'll be able to return takeover from getHUPActionType maintaining the type guarantees

[pipex]

Added a new commit with the proposed change. Included the new jetson xavier versions

[kb2ma]

Good compromise. We always can add some metadata to ActionConfig in the future to differentiate takeover from the others. Let me try building and using this locally on my BoB.

[kb2ma]

Inspected the rework but have not tested yet. Overall looks good except for the comment above on ActionName.

[kb2ma]

Nice comment!

[kb2ma]

Tested a few different ways; works as expected.