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

Project Neokubism #594

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Project Neokubism #594

wants to merge 7 commits into from

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Jul 21, 2021

Based on the spike at https://gitlab.com/teozkr/neokubism/, this turns the VerbParams objects into the "canonical" representation of an action. This means that it becomes possible to extend Client with native-feeling custom subresources, avoids the need for temporary Api objects, and lets us validate that that a given (scope, resource, verb) tuple is legal at compile-time.

It also starts the SNAFUfication (#453) of kube.

This is a pretty huge break-the-world change, but we should be able to keep Api around as a deprecated facade for now.

  • Spike regular verb
  • Spike watch verb
  • Spike websocket verb
  • Implement all verbs
  • Implement verb params
  • Subresources
  • Deprecate untyped Client API
  • Deprecated Api
  • Clean up documentation
  • Changelog
  • Test
  • Clean up old errors
  • Migrate kube-runtime
  • Migrate examples

@nightkr nightkr added the api Api abstraction related label Jul 21, 2021
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

some comments. i do feel this is probably a bit overambitious at the get-go, but i do like the way the scopes and verbs are handled. Also +1 for snafuification.

pub version: &'a str,
}
impl<'a> Verb for ListApiGroupResources<'a> {
type ResponseDecoder = DecodeSingle<k8s_openapi::apimachinery::pkg::apis::meta::v1::APIResourceList>;
Copy link
Member

Choose a reason for hiding this comment

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

now you are getting into discovery - which is arguably a list on the APIResource resource, rather than a separate verb (but legacy resources are awkward).

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I wonder if we could add a special-case impl on List<APIResource> without conflicting with the blanket...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, probably not actually, since List doesn't have the information about the group and version.

Comment on lines +156 to +168
/// Perform a request described by a [`Verb`]
pub async fn call<V: Verb>(&self, verb: V) -> Result<<V::ResponseDecoder as TryFuture>::Ok, CallError>
where
<V::ResponseDecoder as TryFuture>::Error: std::error::Error + Send + Sync + 'static,
{
let req = verb.to_http_request().context(BuildRequestFailed)?;
V::ResponseDecoder::from(self.send(req).await.map_err(Box::new).context(RequestFailed)?)
.into_future()
.await
.map_err(|err| Box::new(err) as Box<dyn std::error::Error + Send + Sync + 'static>)
.context(DecodeFailed)
}

Copy link
Member

Choose a reason for hiding this comment

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

This seems to be the main method here that's causing the abstraction to have to rewrite most of the Client methods.

Would it not make more sense to re-use the existing methods? Or is that going to be too difficult?
Not saying that this setup isn't nice or anything, but it's a big change, and you kind of need a full rewrite-the-world type change to get it in. It might be easier to start with .call just translating from Verb to existing Client::method for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it not make more sense to re-use the existing methods? Or is that going to be too difficult?

The existing methods aren't too useful, and that would cause a circular dependency between Client and Verb. Not insurmountable, but it feels a bit smelly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Besides, everything except request_status and websockets is already implemented with the new ResponseDecoder mechanism, so it wouldn't really save much work to reuse them.

Comment on lines +109 to +111
/// Common query parameters used to select multiple objects
#[derive(Default)]
pub struct Query<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

This does mean we take a sharp break from apimachinery where we literally transcribe the ListOptional, and DeleteOptional into ListParams and DeleteParams. Now we are factoring out bits of that type, when even apimachinery doesn't do that.

Copy link
Member Author

@nightkr nightkr Jul 31, 2021

Choose a reason for hiding this comment

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

True, but the current approach doesn't really translate well anyway (such as the way that we just ignore some ListParams fields on watch, and so on).

Comment on lines +43 to +80
fn poll(mut self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll<Self::Output> {
loop {
break match ready!(self.body.poll_next_unpin(cx)) {
Some(Ok(chunk)) => {
self.chunks.push(chunk);
continue;
}
Some(Err(err)) => Poll::Ready(Err(err).context(ReadFailed)),
None => Poll::Ready(
serde_json::from_reader(BytesVecCursor::from(std::mem::take(&mut self.chunks)))
.context(DeserializeFailed),
),
};
}
}
}

struct BytesVecCursor {
cur_chunk: bytes::buf::Reader<Bytes>,
chunks: std::vec::IntoIter<Bytes>,
}

impl Read for BytesVecCursor {
fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
loop {
break Ok(match self.cur_chunk.read(buf)? {
0 => match self.chunks.next() {
Some(chunk) => {
self.cur_chunk = chunk.reader();
continue;
}
None => 0,
},
n => n,
});
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This does seem to be a significant simplification/restructuring of Client::request_events. It makes me a bit wary. Don't see any references to some of our old error cases like eof.

Copy link
Member Author

Choose a reason for hiding this comment

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

Stream::poll_next_unpin() -> None indicates EOF, and is already handled.

That said, yes, we need to fine-comb for those edge cases before actually merging this. The PR is currently still in more of a prototype phase...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Api abstraction related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants