You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/04/19 16:22:11 UTC
[GitHub] [trafficcontrol] zrhoffman opened a new pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
zrhoffman opened a new pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755
<!--
************ STOP!! ************
If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
-->
## What does this PR (Pull Request) do?
<!-- Explain the changes you made here. If this fixes an Issue, identify it by
replacing the text in the checkbox item with the Issue number e.g.
- [x] This PR fixes #9001 OR is not related to any Issue
^ This will automatically close Issue number 9001 when the Pull Request is
merged (The '#' is important).
Be sure you check the box properly, see the "The following criteria are ALL
met by this PR" section for details.
-->
- [x] This PR fixes #5754 <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- Traffic Monitor
- Traffic Ops
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
Run the TO v1 and TO v2 API tests
## If this is a bug fix, what versions of Traffic Control are affected?
<!-- If this PR fixes a bug, please list here all of the affected versions - to
the best of your knowledge. It's also pretty helpful to include a commit hash
of where 'master' is at the time this PR is opened (if it affects master),
because what 'master' means will change over time. For example, if this PR
fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
look like:
- master (1df853c8)
- 4.0.0
- 4.0.1 (RC1)
If you don't know what other versions might have this bug, AND don't know how
to find the commit hash of 'master', then feel free to leave this section
blank (or, preferably, delete it entirely).
-->
- master (473d9c2422)
- 5.1.2 (RC2)
- 5.1.1 (and all 5.x.x through 5.0.0)
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] Bugfix, documentation is unnecessary, though the release notes for the next release should mention that it is the first release for which Traffic Ops can be used with a ATC 4.1.1 Traffic Monitor.
- [x] This PR includes an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
## Additional Information
<!-- If you would like to include any additional information on the PR for
potential reviewers please put it here.
Some examples of this would be:
- Before and after screenshots/gifs of the Traffic Portal if it is affected
- Links to other dependent Pull Requests
- References to relevant context (e.g. new/updates to dependent libraries,
mailing list records, blueprints)
Feel free to leave this section blank (or, preferably, delete it entirely).
-->
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822842814
> ... so I think the 2nd scenario is already covered.
Oh, I was confused. @ocket8888's comment made it sound like the format was changed as part of v3, but it wasn't. So yeah, the 2nd scenario is definitely covered. Carry on 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619408813
##########
File path: traffic_ops/testing/api/v1/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileName(profileName)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
[No it wouldn't](https://play.golang.org/p/dj8TDpb8Ii5), but my mistake. I checked like 5 times, can't believe I didn't see that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822719692
It's curious that all the CI tests pass; I'm getting this locally:
```
/trafficops-init.sh: line 119: /shared/enroller/federations/010-foo.kabletown.net.json: No such file or directory
```
which I believe based on context probably is because `/shared/enroller/federations` doesn't exist. Shouldn't that be handled automagically? It's just a volume, so I don't think I should have to re-build the enroller container, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822822047
> > Anyway, is the intention of this PR to only change the behavior for the "legacy" monitoring API endpoint? Because in 4.0 I'm still seeing:
>
> I'm not sure why the format change was even necessary in the first place, but this seems like it needs to be fixed in both TO _and_ TM. Basically, TO API 2.x needs to return the original format, and 3.x needs to continue returning the _new_ format. On the TM side, we need to properly "upgrade" the 2.x format into the 3.x format. Currently, it only looks like the "servers" field is upgraded properly from 2.x to 3.x.
>
> Essentially, if we were upgrading from 4.1 to 5.0, we would need to be able to do both:
>
> 1. upgrade TO to 5.0 first, leaving TM at 4.1
>
> 2. upgrade TM to 5.0 first, leaving TO at 4.1
>
>
> If we only fix TO to return the old format at the old API version, that only fixes the 1st scenario.
In order to determine if Traffic Monitor is getting the thresholds, I put a breakpoint in TM's loop over `profile.Parameters.Thresholds` and see if it breaks.
For master at f84ae27eda: https://github.com/apache/trafficcontrol/blob/f84ae27edaaeed65c2ab189bc50cec3a39e97133/traffic_monitor/health/cache.go#L211-L213
For Traffic Monitor at 4.1.1: https://github.com/apache/trafficcontrol/blob/817a702a9de6169c86e30cd70304af380a8e9dc6/traffic_monitor/health/cache.go#L120-L122
With TM at RELEASE-5.0.0 and TO at RELEASE-4.1.1, `profile.Parameters.Thresholds` is non-empty, so I think the 2nd scenario is already covered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822970557
> Well, the `config` section may be the same, but the `trafficServer` section is different, because as of APIv3 the servers will have interfaces
<strike>From what I'm seeing, the Monitoring Config at the latest TO API version has been totally unchanged from 4.1.1 through master at f84ae27eda. It's identical, line for line (after sorting JSON using `jq -S`). The `"trafficServer"` array using TO API 2.0 at RELEASE-4.1.1:
```json
"trafficServers": [
{
"cachegroup": "CDN_in_a_Box_Edge",
"fqdn": "edge.infra.ciab.test",
"hashid": "7825f580-26e7-476f-af8a-8d5adfe7f741",
"hostname": "edge",
"interfaces": [
{
"ipAddresses": [
{
"address": "172.16.239.3",
"gateway": "172.16.239.1",
"serviceAddress": true
},
{
"address": "fc01:9400:1000:8::3",
"gateway": "fc01:9400:1000:8::1",
"serviceAddress": true
}
],
"maxBandwidth": null,
"monitor": true,
"mtu": 1500,
"name": "eth0"
}
],
"port": 80,
"profile": "ATS_EDGE_TIER_CACHE",
"status": "REPORTED",
"type": "EDGE"
},
```
Let me know if you can find any differences, but from my testing, my `diff` is completely empty when diffing the whole Monitoring Config between different TO versions.</strike>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822723975
Rebased onto master so that `/shared/enroller/federations` will exist on this branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619401798
##########
File path: traffic_ops/testing/api/v1/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileName(profileName)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
Yes it is. It's used down here: https://github.com/apache/trafficcontrol/blob/4f368bdc30c847d6b091b1da5114776bd75fa59c/traffic_ops/testing/api/v1/crconfig_test.go#L221
If it was actually unused in that scope, Go would throw a compile error.
##########
File path: traffic_ops/testing/api/v2/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileName(profileName)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
It's used down here: https://github.com/apache/trafficcontrol/blob/4f368bdc30c847d6b091b1da5114776bd75fa59c/traffic_ops/testing/api/v2/crconfig_test.go#L221
##########
File path: traffic_ops/testing/api/v3/crconfig_test.go
##########
@@ -154,6 +156,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByNameWithHdr(cdnName, nil)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileNameWithHdr(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
Here is where it is used: https://github.com/apache/trafficcontrol/blob/4f368bdc30c847d6b091b1da5114776bd75fa59c/traffic_ops/testing/api/v3/crconfig_test.go#L222
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619399019
##########
File path: lib/go-tc/traffic_monitor.go
##########
@@ -424,7 +445,14 @@ type TMParameters struct {
HealthPollingType string `json:"health.polling.type"`
HistoryCount int `json:"history.count"`
MinFreeKbps int64
- Thresholds map[string]HealthThreshold `json:"health_threshold"`
+ Thresholds map[string]HealthThreshold `json:"health_threshold,omitempty"`
+ HealthThresholdJSONParameters
+}
+
+type HealthThresholdJSONParameters struct {
Review comment:
Godocs added in 4f368bdc30.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822724406
I did rebase onto master - I had to because of the shared build stage names thing...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822745311
It looks like for whatever reason rebuilding the enroller was the key to getting the federation thing to work. Odd.
Anyway, is the intention of this PR to only change the behavior for the "legacy" monitoring API endpoint? Because in 4.0 I'm still seeing:
```json
[
{
"name": "CCR_CIAB",
"type": "CCR",
"parameters": null
},
{
"name": "ATS_EDGE_TIER_CACHE",
"type": "EDGE",
"parameters": {
"health.connection.timeout": 2000,
"health.polling.url": "http://${hostname}/_astats?application=&inf.name=${interface_name}",
"health.threshold.availableBandwidthInKbps": ">1750000",
"health.threshold.loadavg": "25.0",
"health.threshold.queryTime": 1000,
"history.count": 30
}
},
{
"name": "ATS_MID_TIER_CACHE",
"type": "MID",
"parameters": {
"health.connection.timeout": 2000,
"health.polling.url": "http://${hostname}/_astats?application=&inf.name=${interface_name}",
"health.threshold.availableBandwidthInKbps": ">1750000",
"health.threshold.loadavg": "25.0",
"health.threshold.queryTime": 1000,
"history.count": 30
}
}
]
```
(from `toget -kpa 4.0 cdns/CDN-in-a-Box/configs/monitoring | jq '.response.profiles'`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619406537
##########
File path: traffic_ops/testing/api/v2/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
Review comment:
Checking the length of `profiles` in 3963215ba4
##########
File path: traffic_ops/testing/api/v3/crconfig_test.go
##########
@@ -154,6 +156,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByNameWithHdr(cdnName, nil)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
Review comment:
Checking the length of `profiles` in 3963215ba4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619406624
##########
File path: traffic_ops/testing/api/v4/crconfig_test.go
##########
@@ -203,6 +205,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName, nil)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
Review comment:
Checking the length of `profiles` in 3963215ba4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822787788
> Anyway, is the intention of this PR to only change the behavior for the "legacy" monitoring API endpoint? Because in 4.0 I'm still seeing:
Looks like API v3 and API v4 already did that. The only endpoints for which the structure of the Health Threshold Parameters changed in #4774 were for API v1 and API v2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822970557
> Well, the `config` section may be the same, but the `trafficServer` section is different, because as of APIv3 the servers will have interfaces
From what I'm seeing, the Monitoring Config at the latest TO API version has been totally unchanged from 4.1.1 through master at f84ae27eda. It's identical, line for line (after sorting JSON using `jq -S`). The `"trafficServer"` array using TO API 2.0 at RELEASE-4.1.1:
```json
"trafficServers": [
{
"cachegroup": "CDN_in_a_Box_Edge",
"fqdn": "edge.infra.ciab.test",
"hashid": "7825f580-26e7-476f-af8a-8d5adfe7f741",
"hostname": "edge",
"interfaces": [
{
"ipAddresses": [
{
"address": "172.16.239.3",
"gateway": "172.16.239.1",
"serviceAddress": true
},
{
"address": "fc01:9400:1000:8::3",
"gateway": "fc01:9400:1000:8::1",
"serviceAddress": true
}
],
"maxBandwidth": null,
"monitor": true,
"mtu": 1500,
"name": "eth0"
}
],
"port": 80,
"profile": "ATS_EDGE_TIER_CACHE",
"status": "REPORTED",
"type": "EDGE"
},
```
Let me know if you can find any differences, but from my testing, my `diff` is completely empty when diffing the whole Monitoring Config between different TO versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822928500
> Hmm I'm noticing that API v1/v2/v3/v4 responses for `cdns/{name}/configs/monitoring` are now identical. Removed the legacy handler in [749ba38](https://github.com/apache/trafficcontrol/commit/749ba38b60825c601cd39cdeb47b8fd7d4d83ac8).
Well, the `config` section may be the same, but the `trafficServer` section is different, because as of APIv3 the servers will have interfaces
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822772371
> Anyway, is the intention of this PR to only change the behavior for the "legacy" monitoring API endpoint? Because in 4.0 I'm still seeing:
I'm not sure why the format change was even necessary in the first place, but this seems like it needs to be fixed in both TO _and_ TM. Basically, TO API 2.x needs to return the original format, and 3.x needs to continue returning the _new_ format. On the TM side, we need to properly "upgrade" the 2.x format into the 3.x format. Currently, it only looks like the "servers" field is upgraded properly from 2.x to 3.x.
Essentially, if we were upgrading from 4.1 to 5.0, we would need to be able to do both:
1. upgrade TO to 5.0 first, leaving TM at 4.1
2. upgrade TM to 5.0 first, leaving TO at 4.1
If we only fix TO to return the old format at the old API version, that only fixes the 1st scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619379037
##########
File path: lib/go-tc/traffic_monitor.go
##########
@@ -424,7 +445,14 @@ type TMParameters struct {
HealthPollingType string `json:"health.polling.type"`
HistoryCount int `json:"history.count"`
MinFreeKbps int64
- Thresholds map[string]HealthThreshold `json:"health_threshold"`
+ Thresholds map[string]HealthThreshold `json:"health_threshold,omitempty"`
+ HealthThresholdJSONParameters
+}
+
+type HealthThresholdJSONParameters struct {
Review comment:
GoDoc?
##########
File path: traffic_ops/testing/api/v2/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileName(profileName)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
same as above RE: variable not used in the scope of its declaration
##########
File path: traffic_ops/testing/api/v4/crconfig_test.go
##########
@@ -203,6 +205,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName, nil)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileName(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
same as above RE: variable not used in the scope of its declaration
##########
File path: traffic_ops/testing/api/v3/crconfig_test.go
##########
@@ -154,6 +156,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByNameWithHdr(cdnName, nil)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
Review comment:
same as above RE: checking `cdns` instead of `profiles`
##########
File path: traffic_ops/testing/api/v1/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileName(profileName)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
`profile` does not appear to be used in this scope
##########
File path: traffic_ops/testing/api/v4/crconfig_test.go
##########
@@ -203,6 +205,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName, nil)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
Review comment:
same as above RE: checking `cdns` instead of `profiles`
##########
File path: traffic_ops/testing/api/v2/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
Review comment:
same as above RE: checking `cdns` instead of `profiles`
##########
File path: traffic_ops/testing/api/v3/crconfig_test.go
##########
@@ -154,6 +156,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByNameWithHdr(cdnName, nil)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByNameWithHdr(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileNameWithHdr(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
same as above RE: variable not used in the scope of its declaration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822822047
> > Anyway, is the intention of this PR to only change the behavior for the "legacy" monitoring API endpoint? Because in 4.0 I'm still seeing:
>
> I'm not sure why the format change was even necessary in the first place, but this seems like it needs to be fixed in both TO _and_ TM. Basically, TO API 2.x needs to return the original format, and 3.x needs to continue returning the _new_ format. On the TM side, we need to properly "upgrade" the 2.x format into the 3.x format. Currently, it only looks like the "servers" field is upgraded properly from 2.x to 3.x.
>
> Essentially, if we were upgrading from 4.1 to 5.0, we would need to be able to do both:
>
> 1. upgrade TO to 5.0 first, leaving TM at 4.1
>
> 2. upgrade TM to 5.0 first, leaving TO at 4.1
>
>
> If we only fix TO to return the old format at the old API version, that only fixes the 1st scenario.
In order to determine if Traffic Monitor is getting the thresholds, I put a breakpoint in `profile.Parameters.Thresholds` and see if it breaks.
For master at f84ae27eda: https://github.com/apache/trafficcontrol/blob/f84ae27edaaeed65c2ab189bc50cec3a39e97133/traffic_monitor/health/cache.go#L211-L213
For Traffic Monitor at 4.1.1: https://github.com/apache/trafficcontrol/blob/817a702a9de6169c86e30cd70304af380a8e9dc6/traffic_monitor/health/cache.go#L120-L122
With TM at RELEASE-5.0.0 and TO at RELEASE-4.1.1, `profile.Parameters.Thresholds` is non-empty, so I think the 2nd scenario is already covered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619406453
##########
File path: traffic_ops/testing/api/v1/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
Review comment:
Checking the length of `profiles` in 3963215ba4.
Removed the redundant condition in d5387d1139.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822723118
> It's curious that all the CI tests pass; I'm getting this locally:
>
> ```
> /trafficops-init.sh: line 119: /shared/enroller/federations/010-foo.kabletown.net.json: No such file or directory
> ```
>
> which I believe based on context probably is because `/shared/enroller/federations` doesn't exist. Shouldn't that be handled automagically? It's just a volume, so I don't think I should have to re-build the enroller container, right?
`/traffic_ops_data/` (copied to `/shared/enroller/`) is a volume between all services, but it is not mounted, the `trafficops` Dockerfile copies it in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619401986
##########
File path: traffic_ops/testing/api/v4/crconfig_test.go
##########
@@ -203,6 +205,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName, nil)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
+ }
+ parameters, _, err := TOSession.GetParametersByProfileName(profileName, nil)
+ if err != nil {
+ t.Fatalf("getting Parameters by Profile name '%s': %s", profileName, err.Error())
+ }
+ parameterMap := map[string]tc.HealthThreshold{}
+ parameterFound := map[string]bool{}
+ const thresholdPrefixLength = len(tc.ThresholdPrefix)
+ for _, parameter := range parameters {
+ if !strings.HasPrefix(parameter.Name, tc.ThresholdPrefix) {
+ continue
+ }
+ parameterName := parameter.Name[thresholdPrefixLength:]
+ parameterMap[parameterName], err = tc.StrToThreshold(parameter.Value)
+ if err != nil {
+ t.Fatalf("converting string '%s' to HealthThreshold: %s", parameter.Value, err.Error())
+ }
+ parameterFound[parameterName] = false
+ }
+ const expectedThresholdParameters = 3
+ if len(parameterMap) != expectedThresholdParameters {
+ t.Fatalf("expected Profile '%s' to contain %d Parameters with names starting with '%s' but %d such Parameters were found", profileName, expectedThresholdParameters, tc.ThresholdPrefix, len(parameterMap))
+ }
+ tmConfig, _, err := TOSession.GetTrafficMonitorConfig(cdnName)
+ if err != nil {
+ t.Fatalf("getting Traffic Monitor Config: %s", err.Error())
+ }
+ profileFound := false
+ var profile tc.TMProfile
Review comment:
Variable used in the scope of its declaration: https://github.com/apache/trafficcontrol/blob/4f368bdc30c847d6b091b1da5114776bd75fa59c/traffic_ops/testing/api/v4/crconfig_test.go#L222
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822862474
Added new Monitoring Config API tests to API v3 and API v4 in b84724764a. Done updating the PR now unless someone finds something to change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822847101
Hmm I'm noticing that API v1/v2/v3/v4 responses for `cdns/{name}/configs/monitoring` are now identical. Removed the legacy handler in 749ba38b60.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822822047
> > Anyway, is the intention of this PR to only change the behavior for the "legacy" monitoring API endpoint? Because in 4.0 I'm still seeing:
>
> I'm not sure why the format change was even necessary in the first place, but this seems like it needs to be fixed in both TO _and_ TM. Basically, TO API 2.x needs to return the original format, and 3.x needs to continue returning the _new_ format. On the TM side, we need to properly "upgrade" the 2.x format into the 3.x format. Currently, it only looks like the "servers" field is upgraded properly from 2.x to 3.x.
>
> Essentially, if we were upgrading from 4.1 to 5.0, we would need to be able to do both:
>
> 1. upgrade TO to 5.0 first, leaving TM at 4.1
>
> 2. upgrade TM to 5.0 first, leaving TO at 4.1
>
>
> If we only fix TO to return the old format at the old API version, that only fixes the 1st scenario.
In order to determine if Traffic Monitor is getting the thresholds, I put a breakpoint in TM's loop over `profile.Parameters.Thresholds` and see if it breaks.
For master at f84ae27eda: https://github.com/apache/trafficcontrol/blob/f84ae27edaaeed65c2ab189bc50cec3a39e97133/traffic_monitor/health/cache.go#L211-L213
For Traffic Monitor at 4.1.1: https://github.com/apache/trafficcontrol/blob/817a702a9de6169c86e30cd70304af380a8e9dc6/traffic_monitor/health/cache.go#L120-L122
With TM at RELEASE-5.0.0 and TO at RELEASE-4.1.1 (rolling back migrations through `20200507000000_interfaces.sql` before snapshotting), `profile.Parameters.Thresholds` is non-empty, so I think the 2nd scenario is already covered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822820313
Ah, so the behavior of `2,0/cdns/{{name}}/monitoring` wasn't matching documentation/expectations?
Then I think you're good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 edited a comment on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822724406
~~I did rebase onto master - I had to because of the shared build stage names thing...~~
Oh, I thought you were telling _me_ to rebase. I'll try again now, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-825647205
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822970557
> Well, the `config` section may be the same, but the `trafficServer` section is different, because as of APIv3 the servers will have interfaces
From what I'm seeing, the Monitoring Config at the latest TO API version has been totally unchanged from 4.1.1 through master at f84ae27eda. It's identical, line for line. The `"trafficServer"` array using TO API 2.0 at RELEASE-4.1.1:
```json
"trafficServers": [
{
"cachegroup": "CDN_in_a_Box_Edge",
"fqdn": "edge.infra.ciab.test",
"hashid": "7825f580-26e7-476f-af8a-8d5adfe7f741",
"hostname": "edge",
"interfaces": [
{
"ipAddresses": [
{
"address": "172.16.239.3",
"gateway": "172.16.239.1",
"serviceAddress": true
},
{
"address": "fc01:9400:1000:8::3",
"gateway": "fc01:9400:1000:8::1",
"serviceAddress": true
}
],
"maxBandwidth": null,
"monitor": true,
"mtu": 1500,
"name": "eth0"
}
],
"port": 80,
"profile": "ATS_EDGE_TIER_CACHE",
"status": "REPORTED",
"type": "EDGE"
},
```
Let me know if you can find any differences, but from my testing, my `diff` is completely empty when diffing the whole Monitoring Config between different TO versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822973038
Never mind, forgot to snap. Re-adding the legacy handler...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman edited a comment on pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#issuecomment-822822047
> > Anyway, is the intention of this PR to only change the behavior for the "legacy" monitoring API endpoint? Because in 4.0 I'm still seeing:
>
> I'm not sure why the format change was even necessary in the first place, but this seems like it needs to be fixed in both TO _and_ TM. Basically, TO API 2.x needs to return the original format, and 3.x needs to continue returning the _new_ format. On the TM side, we need to properly "upgrade" the 2.x format into the 3.x format. Currently, it only looks like the "servers" field is upgraded properly from 2.x to 3.x.
>
> Essentially, if we were upgrading from 4.1 to 5.0, we would need to be able to do both:
>
> 1. upgrade TO to 5.0 first, leaving TM at 4.1
>
> 2. upgrade TM to 5.0 first, leaving TO at 4.1
>
>
> If we only fix TO to return the old format at the old API version, that only fixes the 1st scenario.
In order to determine if Traffic Monitor is getting the thresholds, I put a breakpoint in TM's loop over `profile.Parameters.Thresholds` and see if it breaks.
For Traffic Monitor at master@[`f84ae27`](https://github.com/apache/trafficcontrol/commit/f84ae27edaaeed65c2ab189bc50cec3a39e97133): https://github.com/apache/trafficcontrol/blob/f84ae27edaaeed65c2ab189bc50cec3a39e97133/traffic_monitor/health/cache.go#L211-L213
For Traffic Monitor at 4.1.1: https://github.com/apache/trafficcontrol/blob/817a702a9de6169c86e30cd70304af380a8e9dc6/traffic_monitor/health/cache.go#L120-L122
With TM at RELEASE-5.0.0 and TO at RELEASE-4.1.1 (rolling back migrations through `20200507000000_interfaces.sql` before snapshotting), `profile.Parameters.Thresholds` is non-empty, so I think the 2nd scenario is already covered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5755: Return legacy Monitoring Config from legacy Monitoring Config handler
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5755:
URL: https://github.com/apache/trafficcontrol/pull/5755#discussion_r619383162
##########
File path: traffic_ops/testing/api/v1/crconfig_test.go
##########
@@ -153,6 +155,89 @@ func UpdateTestCRConfigSnapshot(t *testing.T) {
}
}
+func MonitoringConfig(t *testing.T) {
+ if len(testData.CDNs) < 1 {
+ t.Fatalf("no cdn test data")
+ }
+ const cdnName = "cdn1"
+ const profileName = "EDGE1"
+ cdns, _, err := TOSession.GetCDNByName(cdnName)
+ if err != nil {
+ t.Fatalf("getting CDNs with name '%s': %s", cdnName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a CDN named %s", cdnName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 CDN named %s but found %d CDNs", cdnName, len(cdns))
+ }
+ profiles, _, err := TOSession.GetProfileByName(profileName)
+ if err != nil {
+ t.Fatalf("getting Profiles with name '%s': %s", profileName, err.Error())
+ }
+ if len(cdns) < 1 {
+ t.Fatalf("expected to find a Profile named %s", profileName)
+ }
+ if len(cdns) > 1 {
+ t.Fatalf("expected exactly 1 Profiles named %s but found %d Profiles", profileName, len(profiles))
Review comment:
I'm not sure why this didn't make it into my review, but it was supposed to:
This seems like it should be checking the length of `profiles`, not `cdns`.
Also, I think if you remove the `len(thing) < 1` blocks and change the `len(thing) > 1` blocks' conditions to `len(thing) != 1` the error message therein will suffice to cover both cases. It's too late to save some typing now, but I thought I'd point it out in case you wanna change it anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org