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

Assign multiple servers to a capability #7079

Merged

Conversation

rimashah25
Copy link
Contributor

@rimashah25 rimashah25 commented Sep 21, 2022

Closes: #6033


Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Ops
  • Traffic Portal

What is the best way to verify this PR?

Using API test the new route with a list of servers for a given server capability

Success Assignment
curl -X POST -k -H "Accept: application/json" --cookie "mojolicious=" --data @mspc.json https://localhost:port/api/4.1/multiple_servers_capabilities/

{
	"alerts": [{
		"text": "Multiple Servers assigned to a capability",
		"level": "success"
	}],
	"response": {
		"serverCapabilities": ["eas"],
		"serverIds": [3, 2]
	}
}

Incorrect route
curl -X POST -k -H "Accept: application/json" --cookie "mojolicious==" --data @mspc.json https://localhost:port/api/4.0/multiple_servers_capabilities

{
	"alerts": [{
		"text": "The requested path '/api/4.0/multiple_servers_capabilities' does not exist.",
		"level": "error"
	}]
}

Using TP

  1. Once you create a server capability, click on created server capability (eg: disk), and then click on Manage Servers (changed the name from View Servers)
  2. To assign multiple servers to a capability, click on image,
    image
  3. A new dialog window listing all servers (and the one assigned to the existing server as checked) should open up, like this:

image

  1. You can chose from the list and click submit and then see it update on the server/<server_name>/capabilties table.

If this is a bugfix, which Traffic Control versions contained the bug?

PR submission checklist

@rimashah25 rimashah25 marked this pull request as ready for review September 22, 2022 16:22
@rimashah25 rimashah25 force-pushed the assign-multiple-servers-to-capability branch from 4f9e38d to 8804e91 Compare September 27, 2022 02:21
Copy link
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

I know this would probably be a large change at this point, but why not just augment the existing /multiple_server_capabilities route to accept an array of both server IDs and Capability names instead of making a whole new endpoint?

@rimashah25 rimashah25 force-pushed the assign-multiple-servers-to-capability branch from d78f3a8 to dbf8b36 Compare September 29, 2022 18:36
@rimashah25 rimashah25 requested review from srijeet0406 and ocket8888 and removed request for srijeet0406 and ocket8888 September 29, 2022 18:43
@rimashah25 rimashah25 force-pushed the assign-multiple-servers-to-capability branch from 0507ae2 to 03d684e Compare October 4, 2022 00:09
@rimashah25 rimashah25 force-pushed the assign-multiple-servers-to-capability branch from 4d017cd to 7655d5c Compare October 5, 2022 21:15
@rimashah25 rimashah25 force-pushed the assign-multiple-servers-to-capability branch from 3307a37 to c120f87 Compare October 10, 2022 15:03
@rimashah25
Copy link
Contributor Author

Sorry just seeing this, where are those checks? I usually do a POST followed by a GET to make sure whether or not the operation actually went through to the database or not.

The way new integration tests are written, one doesn't need do that.

@ocket8888 ocket8888 added new feature A new feature, capability or behavior Traffic Ops related to Traffic Ops Traffic Portal related to Traffic Portal medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Portal v2 Related to the experimental Traffic Portal v2 labels Oct 11, 2022
@rimashah25 rimashah25 force-pushed the assign-multiple-servers-to-capability branch from f4cc956 to bdf32e1 Compare October 27, 2022 05:49
@rimashah25 rimashah25 force-pushed the assign-multiple-servers-to-capability branch from 38fc596 to 35f6e8e Compare October 27, 2022 15:03
Copy link
Contributor

@srijeet0406 srijeet0406 left a comment

Choose a reason for hiding this comment

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

Manual testing looks good. I'm just mentioning this here so that we have a comment to refer to in the future. If you hit the POST endpoint with multiple server IDs and one server capability, but the pageType is set to server, it will still just associate the one server capability to just the FIRST server in that list (since the pageType is server).
Similarly, if you hit the same endpoint with multiple server capabilities and just one server ID, but with the pageType set to sc, it will assign ONLY THE FIRST server capability to the server.

The above is only applicable for users hitting the API directly, because TP adds these checks before passing in the request.

Otherwise, all many to many assignments fail as expected.
Any pageType other than server and sc fails as expected.
A blank pageType fails as expected.

I'll merge it once GHA tasks pass.

@srijeet0406 srijeet0406 merged commit 7086637 into apache:master Oct 31, 2022
@rimashah25 rimashah25 deleted the assign-multiple-servers-to-capability branch October 31, 2022 20:02
@asf-ci asf-ci mentioned this pull request Nov 1, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium impact impacts a significant portion of a CDN, or has the potential to do so new feature A new feature, capability or behavior Traffic Ops related to Traffic Ops Traffic Portal v2 Related to the experimental Traffic Portal v2 Traffic Portal related to Traffic Portal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to assign and remove a server capability for multiple servers at a time
3 participants