Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WiP] CP-26340: SR.probe changes #208

Closed
wants to merge 2 commits into from
Closed

Conversation

edwintorok
Copy link
Contributor

@edwintorok edwintorok commented Feb 26, 2018

SMAPIv3 returns a list of configurations:

  • suitable for SR.attach together with sr_info
  • suitable for SR.create
  • suitable as further input to SR.probe (recursively)

SMAPIv1 is unaffected, it always returns a Raw result.

Has to be merged together with:
xapi-project/xapi-storage#74
xapi-project/xapi-storage-script#58
xapi-project/xen-api#3477

This is meant for discussion, we should merge only after the xapi-storage-plugins implementation is done, and we've agreed how the final API should look like.

In particular the SMAPIv3 plugin returns this type:
https://github.com/xapi-project/xapi-storage/blob/feature/REQ477/probe/generator/lib/control.ml#L59-L74

Should we make it return something more similar to the type here in xcp-idl? And what should we do with extra_info, should we change the probed device_config type to:

type probed_device_config = {
  configuration: (string * string) list; (* plugin-specific configuration suitable for SR.create, SR.attach, SR.probe *)
  extra_info : (string * string) list; (* Additional plugin-specific information about this configuration, that might be of use for an API user. This can for example include the LUN or the WWPN." *)
}

@coveralls
Copy link

Coverage Status

Coverage remained the same at 61.753% when pulling 4a1fc3e on feature/REQ477/probe into ed14734 on master.

@coveralls
Copy link

coveralls commented Feb 26, 2018

Coverage Status

Coverage remained the same at 61.753% when pulling 91180d7 on feature/REQ477/probe into ed14734 on master.

type probe = {
srs: (string * sr_info) list; (* SRs we found *)
uris: string list; (* other uris we found which could be probed recursively *)
attachable: (device_config * sr_info) list; (* Attachable SRs we found *)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, a question about the naming here: since in SMAPIv3 SM returns an sr for these SRs, which is the same type that's returned by SR.attach, does that mean that these SRs are already attached?
In SMAPIv3, the interface allows us to use these srs returned by probe to do operations like SR.ls. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point, @MarkSymsCtx: does probe need to list already attached SRs differently from SRs that are present and not attached already?
Should we have 4 categories of probe results then?

  • incomplete
  • creatable
  • attached
  • introduceable/attachable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise someone could take an SR returned by probe, run SR.ls on it as @gaborigloi pointed out and get an error because its not actually mounted anymore...

Copy link
Contributor

@gaborigloi gaborigloi Feb 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe we could return 4 lists of probe results. Or just have a variable which can be one of the four categories.

Copy link
Contributor

@gaborigloi gaborigloi Feb 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the above reasons we should definitely not return an sr for an existing SR that isn't attached. I think should indicate differently that there is an existing SR with this configuration.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the plugin probe code is unlikely to be able to determine the difference between an attached SR and a detached but attachable SR give the input provided to it via probe.

Copy link
Contributor

@gaborigloi gaborigloi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one question about the naming - maybe I misunderstood what these new SMAPIv3 fields stand for.

edwintorok and others added 2 commits March 7, 2018 12:12
SMAPIv3 returns a list of configurations:
* suitable for SR.attach together with sr_info
* suitable for SR.create
* suitable as further input to SR.probe (recursively)

SMAPIv1 is unaffected, it always returns a `Raw` result.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
@edwintorok
Copy link
Contributor Author

Closing, will reopen when probe branch is complete

@edwintorok edwintorok closed this Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants