Circular imports when implementing Redfish schema 1.16.0

[pallasscat]

Hi @stmcginnis,

Sorry to bother you again.

I'm working on Redfish Storage v1.13.0 and have encountered a circular import issue.

redfish/drive references swordfish/volume (including many other entities):

"Volumes": {
     "description": "An array of links to the volumes that this drive either wholly or only partially contains.",
     "$ref": "http://redfish.dmtf.org/schemas/swordfish/v1/Volume.json#/definitions/Volume",
     ...
}

which, in turn, references redfish/drive:

"Drives": {
    "description": "An array of references to the drives which contain this volume. [...]",
    "items": {
        "$ref": "http://redfish.dmtf.org/schemas/v1/Drive.json#/definitions/Drive"
    },
    "type": "array",
    ...
},

Redfish schema version 1.16.0 has a lot of these cross-references, which ends up being a pain. Do you have any suggestions on re-structuring the library?

[stmcginnis]

Yeah, it's really kind of a pain how Swordfish builds on Redfish, but then Redfish decided to pick up some of the definitions from Swordfish.

The common module has been used to try to help with this. Things that are both Redfish and Swordfish can go under there to help break that circular dependency.

That's probably the best place to put the drive and volume definitions, or we could maybe add another storage top level module to make it clear.

[pallasscat]

common seems like a fix, but I'm afraid that we'd end up with common consisting of 50:50 redfish and swordfish.

I had an idea of prioritizing Redfish over Swordfish and leaving Swordfish entities without any methods other than UnmarshalJSON (and leave them as "terminal" entities without any references). That would help with situations where redfish entity references swordfish entity that has methods referencing original redfish entity; but would reduce functionality of swordfish module. Common stuff could be moved from redfish to common.

I personally use read-only functionality of Redfish (Prometheus exporter) and don't care about Swordfish at all; but that's my use case.

[stmcginnis]

but would reduce functionality of swordfish module

I'm really not sure if anyone is using the Swordfish functionality at this point, but a lot of the changes for storage are only going in to Swordfish. So I would hate to limit swordfish just to keep things convenient for redfish.

That said, since we only really know of Redfish users right now, I'm not too opposed to that. We can always refactor behind the scenes later if there is someone that is trying to use some of the swordfish functionality but can't with the current implementation.

I guess I'd have to see the code too, but the idea sounds reasonable. Thanks for thinking about a good way to structure this!

[pallasscat]

So I would hate to limit swordfish just to keep things convenient for redfish.

Same here. In theory, the likelyhood of someone jumping from redfish/drive to swordfish/volume to redfish/drive is really low, though I don't have anything Swordfish-capable handy and I can't confirm that.

Thanks for thinking about a good way to structure this!

Thank you, but I don't expect my input to be that valuable :) I'm on junior level, I guess (I'm a sysadmin); and my coding is normally limited to scripting or simple apps <2k SLOC. This is the first lib I'm trying to work on.

[pallasscat]

Ideas:

1. Create additional directories for each Redfish / Swordfish schemadoc; e.g.

redfish/drive/drive.go
redfish/volume/volume.go
swordfish/drive/drive.go

Does not resolve the issue completely ( redfish/drive - swordfish/volume - redfish/drive ), but adds granularity to imports.

2. As discussed previously, make Swordfish entities terminal and remove Redfish entities from their methods (replace with json.RawMessage in case someone wants to get them).

Resolves the issue, but reduces Swordfish functionality.

3. Dump redfish and swordfish into a single package and prefix colliding entities with Swordfish.

Resolves the issue, makes a huge mess.

4. As you suggested, start moving entities to common.

Resolves the issue, may make things more complicated.

5. import "unsafe" and //go:linkname

Dirty hack.

I'd personally go with numbers 1 and 2 - looks like a future-proof solution until DMTF and SNIA figure out what definitions they'd like to use for what. drive-volume-drive is the only loop I was able to find so far.

[stmcginnis]

Thanks for thinking this through and writing this up!

Another option would be to just define these under both redfish and swordfish. This adds some duplication, but does feel like it might be the right approach. It would keep things cleanly separated and users of the library would just find things where they expect them to be.

There's slightly more work on the development side to maintain things in both places.

Otherwise I agree - 1 or 2 would be the best of those options.

[pallasscat]

to just define these under both redfish and swordfish

I'm sorry, but can you elaborate?

[stmcginnis]

Right now we have the circular dependency because redfish/drive references swordfish/volume. We could duplicate the volume definition so that there are both redfish/volume and swordfish/volume definitions so each package only refers to its own definition of the objects.

That's not how the spec is defined, but we could implement it that way in the library to break the circular references.

Hopefully that makes sense. I have not looked as closely as you have yet, so I trust whatever you think would be the best way forward. My main concern is making sure these internal details are kept internally and however it is addressed doesn't put extra burden on consumers of the library.

[pallasscat]

I thought of that as well, however we already have swordfish/volume and redfish/volume defined. They are both Volume, they represent the same logical object volume, but they are different for Redfish and Swordfish (though some fields match).

[stmcginnis]

If I remember right, the redfish/volume was deprecated and the intention was to start using the swordfish/volume definition in the specs. But either way, just wondering if we need to do some internal work to try to make them less different, at least as far as consumption.

Sorry, I should really go back and read the spec again and look at the code, but I haven't been able to get the time lately. :]

[pallasscat]

You are correct, I completely missed "$id": "http://redfish.dmtf.org/schemas/swordfish/v1/Volume.json" :(

Looking at it right now, I may be wrong and the idea of combining redfish and swordfish into a single package redfish is better!

[pallasscat]

I've just dumped all of the contents of swordfish into redfish; changed package swordfish to package redfish, removed some unnecessary redfish. references... and all tests passed.

[pallasscat]

@stmcginnis made a branch where swordfish/ is merged with redfish/; no other updates.