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

Add Block Storage API #151

Merged
merged 58 commits into from
Apr 5, 2024
Merged

Add Block Storage API #151

merged 58 commits into from
Apr 5, 2024

Conversation

gierens
Copy link
Contributor

@gierens gierens commented Jan 25, 2024

This starts adding the block storage API alias cinder. So far it implements rudimentary versions of list_volumes, find_volumes and get_volume with an equally minimal Volume struct. But I does compile and returns things.

Although the cinder API is very similar to nova's API, this is currently closer to the image module. For example, at the moment it goes directly to the /volumes/detail endpoint, rather than defining some kind of VolumeSummary for /volumes and offering a detailed() as the compute module does.

This is for simplicity, just to get things rolling. I need this at work, so I can invest a fair bit of time. I'll make it more consistent along the way.

@dtantsur
Copy link
Owner

dtantsur commented Mar 8, 2024

@gierens is this change still a draft? It looks pretty good to me. Could use a full acceptance test, and at least some unit test failures seem real (I need to check the others).

@gierens
Copy link
Contributor Author

gierens commented Mar 9, 2024

@gierens is this change still a draft? It looks pretty good to me.

Yeah, I was a little busy the last couple of weeks, that's why I didn't add anything more up until now. But I wanted to at least provide the most important volume functions before turning it into a normal PR. And then for snapshots and other stuff, this could go into a follow-up PR.

Could use a full acceptance test, and at least some unit test failures seem real (I need to check the others).

I will now continue to work on this, I just added volume deletion and can also take a look at the failing jobs.

@gierens
Copy link
Contributor Author

gierens commented Mar 16, 2024

I just added volume creation and with that I guess the most important functionality is present. On top of this I can now write a few integration tests and take a look at the still failing CI jobs. But after that the PR should be ready for review.

@dtantsur
Copy link
Owner

The No floating IP issue has been haunting the project for a long time. It's possible that I'm simply too optimistic when assuming how long it takes to allocate such an IP. Chances are high, your patch is not at fault.

@dtantsur
Copy link
Owner

Let's see if #153 provides any improvement.

@dtantsur
Copy link
Owner

@gierens if you rebase the patch, you'll pick up various CI fixes. Everything that fails afterwards is probably on you.

gierens added 19 commits March 16, 2024 18:16
Signed-off-by: Sandro-Alessio Gierens <[email protected]>
Signed-off-by: Sandro-Alessio Gierens <[email protected]>
Signed-off-by: Sandro-Alessio Gierens <[email protected]>
….inner

This allows to hide session details and just focus on the wrapped inner
protocol::Volume.

Signed-off-by: Sandro-Alessio Gierens <[email protected]>
@dtantsur
Copy link
Owner

Great job, thank you! I've left a few comments inline, could you check them?

Please let me know when I reach the point of abusing your patience, I can fix minor issues myself then :)

gierens added 10 commits March 18, 2024 08:17
Signed-off-by: Sandro-Alessio Gierens <[email protected]>
We are not actually sure what this field does or how it is different
from the migration_status, and the naming is a little uninformative.
So, we simply skip it for now.

Signed-off-by: Sandro-Alessio Gierens <[email protected]>
@gierens gierens requested a review from dtantsur March 23, 2024 17:14
@gierens
Copy link
Contributor Author

gierens commented Mar 23, 2024

@dtantsur Alright, I think I have addressed all comments and for the more complex cases as with the DateTime I also provided some info about how I solved them in my answers to the respective comments.

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum DateTime {
WithTz(chrono::DateTime<chrono::FixedOffset>),
WithoutTz(chrono::NaiveDateTime),
Copy link
Owner

Choose a reason for hiding this comment

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

Won't this actually mean UTC?

pub snapshot_id: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub backup_id: Option<String>,
pub name: String, // not optional in spec, but doesn't work with None/null, only with ""
Copy link
Owner

Choose a reason for hiding this comment

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

Could be a helper for serialize_with (to convert None to "")

transparent_property! {
#[doc = "Migration status."]
migration_status: ref Option<String>
}
Copy link
Owner

Choose a reason for hiding this comment

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

Properties should be sorted here for easier navigation in the docs.

@dtantsur dtantsur merged commit 318cba3 into dtantsur:master Apr 5, 2024
15 checks passed
@dtantsur
Copy link
Owner

dtantsur commented Apr 5, 2024

@gierens I hope you don't mind me squashing the commits: 58 may easily be more than we had in rust-openstack before that :)

Let's continue the discussion around DateTime? I have a strong feeling that when they don't provide a timezone, they actually mean UTC. Could you try confirming?

I'll hold releasing the changes until we solve the DateTime problem and maybe figure out enums.

@gierens
Copy link
Contributor Author

gierens commented Apr 6, 2024

@dtantsur That's totally fine. I tend to do atomic commits because squashing is a lot easier than taking commits apart. I probably could've done an interactive rebase in the end to cleanup the history a bit. But now that this one is in we can work on the rest in smaller chunks/PRs.

About the DateTime, yeah, I do share that suspicion. It's unfortunate the API reference is this vague. I'll experiment around a bit and get back to you then.

About releasing, sure totally understandable.

milliams pushed a commit to milliams/rust-openstack that referenced this pull request Apr 8, 2024
This starts adding the block storage API alias cinder. So far it implements rudimentary versions of list_volumes, find_volumes and get_volume with an equally minimal Volume struct. But I does compile and returns things.

Although the cinder API is very similar to nova's API, this is currently closer to the image module. For example, at the moment it goes directly to the /volumes/detail endpoint, rather than defining some kind of VolumeSummary for /volumes and offering a detailed() as the compute module does.

This is for simplicity, just to get things rolling. I need this at work, so I can invest a fair bit of time. I'll make it more consistent along the way.
milliams pushed a commit to milliams/rust-openstack that referenced this pull request Apr 14, 2024
This starts adding the block storage API alias cinder. So far it implements rudimentary versions of list_volumes, find_volumes and get_volume with an equally minimal Volume struct. But I does compile and returns things.

Although the cinder API is very similar to nova's API, this is currently closer to the image module. For example, at the moment it goes directly to the /volumes/detail endpoint, rather than defining some kind of VolumeSummary for /volumes and offering a detailed() as the compute module does.

This is for simplicity, just to get things rolling. I need this at work, so I can invest a fair bit of time. I'll make it more consistent along the way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants