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

Support for commit-check and commit-check-result #497

Open
riccardo-roveri-labs opened this issue Feb 17, 2025 · 13 comments
Open

Support for commit-check and commit-check-result #497

riccardo-roveri-labs opened this issue Feb 17, 2025 · 13 comments

Comments

@riccardo-roveri-labs
Copy link

I did not find functions to call /api/blueprints/{blueprint_id}/commit-check and /api/blueprints/{blueprint_id}/commit-check-result .

Is it possible to implement them in the library? They seem fairly simple endpoint.

Thanks,
Riccardo Roveri

@chrismarget-j
Copy link
Collaborator

I'm not familiar with this endpoint, but I've just had a peek at the documentation.

Is it the case that you'd POST to commit-check, and the GET at commit-check-result? Is there a race condition to worry about here? I'm not sure how you can be sure that the payload retrieved via GET is the result of any specific check.

Maybe this doesn't matter?

@riccardo-roveri-labs
Copy link
Author

I imagined it as two different functions. The time it takes to check is in the order of seconds / tens of seconds in my case, so in any case i would not aggregate them in a single call.

For me a simple wrapper for the two calls is enough.

@chrismarget-j
Copy link
Collaborator

chrismarget-j commented Feb 17, 2025

I'm wondering about the general utility/safety of this sort of function in an environment where multiple applications may be meddling with the fabric at the same time.

Maybe we can make it atomic-ish with some locking? Returning a "result" when the specific query to which it applies is ambiguous makes me a little nervous.

Ideally, check would produce a transaction ID and check-result would make it clear that the result is describing your check and not some check performed at a later (or earlier?) time.

Or we throw up our hands and document the behavior :)

I'll talk to the backend folks about potential pitfalls and follow up here.

edit: How are you able to tell how long the check takes? Is that the time for a response when POSTing to the check endpoint?

@chrismarget-j
Copy link
Collaborator

Oh, we recently added Client.DoRawJsonTransaction(). You can probably use this thing to interact with those endpoints in the short term.

@rajagopalans
Copy link
Collaborator

rajagopalans commented Feb 17, 2025 via email

@riccardo-roveri-labs
Copy link
Author

The problem is in the API itself, checking a blueprint is an asynchronous operation. This is handled in the UI by polling the blueprint status.

The result of check-result includes the following structure:
{ systems: {} // not interesting for this purpose blueprint: { state: enum [failure | empty | success | pending | timeout | rejected | unsupported | unknown | unloadable | unavailable] blueprint_version: int validity: enum [fresh | unknown | stale] } }

You can check if is not pending anymore and the validity is fresh you can be sure that nobody changed it. Another way, maybe more reliable, is to check the blueprint_version before calling check and verifying that is the same as in the response.

In any case you need to poll for the check-result state to update. I do not know if polling is an expected behavior, and how can it be handled correctly (maybe is already done in some places). For sure i would implement both a atomic and non-atomic version of the API.

PS: Thanks for the suggestion about Client.DoRawJsonTransaction()

@riccardo-roveri-labs
Copy link
Author

Out of curiosity, what are you using this API for?

The response was incomplete because i fat-fingered :P

I want to check if the blueprint status valid before committing. I believe that Apstra checks it anyway before applying it to the devices but i want to catch it earlier. Hope it makes sense

@chrismarget-j
Copy link
Collaborator

handled in the UI by polling the blueprint status

User hits the "check" button in the UI, and the UI's background polling eventually surfaces any errors which may exist?

I think if we implemented this in the SDK, we'd want to idiot-proof it using one of the approaches you mentioned. Would that be over-stepping in your opinion?

@riccardo-roveri-labs
Copy link
Author

riccardo-roveri-labs commented Feb 17, 2025

User hits the "check" button in the UI, and the UI's background polling eventually surfaces any errors which may exist?

Yes exactly, it first stays in a loading state then shows the user the new state by polling the API in the background.

I think if we implemented this in the SDK, we'd want to idiot-proof it using one of the approaches you mentioned. Would that be over-stepping in your opinion?

I am too inexperienced in this world to give you an answer. In my opinion if you are using this library you should know what are you doing, otherwise stick to the UI.

I have some doubt about polling, does not feel right to me, are you already doing it in other functions? Maybe a less ergonomic but safer option would be to make the user pass the version of the blueprint. If in the check-result the field is different give an error. But it would not be atomic, as you wanted.

@chrismarget-j
Copy link
Collaborator

polling ... are you already doing it in other functions?

Yes.

Take this transaction which I happened to have on my screen from some recent experiment. It's a snapshot from Wireshark:

Image
  • In packet 266 we do PATCH as a part of some SDK method or other.
  • The response in 273 contains a task ID (you can't see it in this summary). The work isn't done, it's been offloaded to a worker thread somewhere within Apstra.
  • In 275 we query for the task status.
  • The response in 279 indicates the task is not complete.
  • We query again and In the response to 297 we learn that the task has completed.
  • In 322 we request the actual API response to the thing we sent in 266.
  • The response to our PATCH is delivered in 333.

So, yeah. There's a bunch of polling handled behind the scenes in the SDK.

@riccardo-roveri-labs
Copy link
Author

If the user understands if is the ideal solution. I think anyone using this SDK would be doing the same with the raw api, fire a check then polling for check-result.

I am no go expert but maybe you can fire this in a goroutine and return a chan that the user waits on for the result so is obvious that is async. I would also throw an error if the result has the field validity different from fresh.

@chrismarget-j
Copy link
Collaborator

I whipped up a dumb example of using the raw json method in case it proves helpful:

func TestFoo(t *testing.T) {
	ctx := context.Background()

	cfg := apstra.ClientCfg{
		Url:  "https://apstra.example.com",
		User: "redacted",
		Pass: "redacted",
	}

	client, err := cfg.NewClient(ctx)
	if err != nil {
		t.Fatal(err)
	}

	blueprintId := "d4199834-e7d0-4795-9f16-4f9ed059d37a"
	ccrUrl, err := url.Parse(fmt.Sprintf("/api/blueprints/%s/commit-check-result", blueprintId))

	req := apstra.RawJsonRequest{
		Method: http.MethodGet,
		Url:    ccrUrl,
	}

	var resp struct {
		Blueprint struct {
			State string `json:"state"`
		} `json:"blueprint"`
	}

	err = client.DoRawJsonTransaction(ctx, req, &resp)
	if err != nil {
		t.Fatal(err)
	}

	t.Log(resp.Blueprint.State)
}

@chrismarget-j
Copy link
Collaborator

Update: I've talked with engineering folks and understand this API endpoint much better now.

I think we'll be able to deliver something useful.

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

No branches or pull requests

3 participants