You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/07/21 17:57:07 UTC
[GitHub] [trafficserver] rob05c opened a new pull request #7023: Add nexthop strategy plugins
rob05c opened a new pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023
Adds a remap plugin hook to add a nexthop strategy object, and moves and adds other necessary API symbols necessary to implement strategies in plugins.
Also adds an example nexthop strategy plugin.
This adds the following to the API:
```
enum TSHostStatus
enum TSHostStatusReason
enum TSParentResultType
class TSNextHopSelectionStrategy
struct TSParentResult
const char *ParentResultStr
struct TSParentResult
bool TSHostnameIsSelf(const char* hostname)
bool TSHostStatusGet(const char *hostname, TSHostStatus* status, unsigned int *reason)
void TSHostStatusSet(const char *hostname, TSHostStatus status, const unsigned int down_time, const unsigned int reason)
struct TSParentResult* TSHttpTxnParentResultGet(TSHttpTxn txnp)
TSReturnCode TSRemapInitStrategy(TSNextHopSelectionStrategy *&strategy, void *ih, char *errbuf, int errbuf_size)
```
I tried to minimize API changes, but I didn't see a way to do it any smaller.
The example plugin is also a lot of duplication from the core ConsistentHash strategy. I didn't see a reasonable way to reduce that either, while keeping the ConsistentHash strategy in core for now.
I'm definitely open to suggestions.
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r500315024
##########
File path: proxy/ParentSelection.cc
##########
@@ -1869,23 +1871,65 @@ void
show_result(ParentResult *p)
{
switch (p->result) {
- case PARENT_UNDEFINED:
- printf("result is PARENT_UNDEFINED\n");
+ case TS_PARENT_UNDEFINED:
+ printf("result is TS_PARENT_UNDEFINED\n");
break;
- case PARENT_DIRECT:
- printf("result is PARENT_DIRECT\n");
+ case TS_PARENT_DIRECT:
+ printf("result is TS_PARENT_DIRECT\n");
break;
- case PARENT_SPECIFIED:
- printf("result is PARENT_SPECIFIED\n");
+ case TS_PARENT_SPECIFIED:
+ printf("result is TS_PARENT_SPECIFIED\n");
printf("hostname is %s\n", p->hostname);
printf("port is %d\n", p->port);
break;
- case PARENT_FAIL:
- printf("result is PARENT_FAIL\n");
+ case TS_PARENT_FAIL:
+ printf("result is TS_PARENT_FAIL\n");
break;
default:
// Handled here:
// PARENT_AGENT
break;
}
}
+
+void
+ParentResult::copyFrom(TSParentResult *r)
+{
+ this->hostname = r->hostname;
+ this->port = r->port;
+ this->retry = r->retry;
+ this->result = r->result;
+ memcpy(this->chash_init, r->chash_init, TS_MAX_GROUP_RINGS * sizeof(bool));
+ this->first_choice_status = r->first_choice_status;
+ this->line_number = r->line_number;
+ this->last_parent = r->last_parent;
+ this->start_parent = r->start_parent;
+ this->last_group = r->last_group;
+ this->wrap_around = r->wrap_around;
+ memcpy(this->mapWrapped, r->mapWrapped, 2 * sizeof(bool));
+ this->last_lookup = r->last_lookup;
+ for (int i = 0; i < TS_MAX_GROUP_RINGS; ++i) {
+ this->chashIter[i] = r->chashIter[i];
+ }
+}
+
+void
+ParentResult::copyTo(TSParentResult *r)
Review comment:
Any reason these aren't assignment operators?
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469250004
##########
File path: include/ts/ts.h
##########
@@ -29,6 +29,7 @@
#pragma once
+#include <string.h>
Review comment:
Why is this needed?
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r500326325
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -9790,6 +9791,54 @@ TSHttpTxnRedoCacheLookup(TSHttpTxn txnp, const char *url, int length)
return TS_ERROR;
}
+TSReturnCode
+TSHostnameIsSelf(const char *hostname)
+{
+ const bool isSelf = Machine::instance()->is_self(hostname);
+ if (isSelf) {
+ return TS_SUCCESS;
+ }
+ return TS_ERROR;
+}
+
+TSReturnCode
+TSHostStatusGet(const char *hostname, TSHostStatus *status, unsigned int *reason)
Review comment:
As a general rule, I prefer the C API functions to take strings as `const char *, int` which is commonly done in (for instance) the MIME header functions. This makes working with `string_view` (or `TextView`) in plugins much easier. Internally `HostStatus::getHostStatus` should take an `string_view` parameter so that it can be directly passed from this API call.
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r506571882
##########
File path: include/ts/ts.h
##########
@@ -2573,6 +2573,38 @@ tsapi TSIOBufferReader TSHttpTxnPostBufferReaderGet(TSHttpTxn txnp);
*/
tsapi TSReturnCode TSHttpTxnServerPush(TSHttpTxn txnp, const char *url, int url_len);
+/*
+ * Returns TS_SUCCESS if hostname is this machine, as used for parent and remap self-detection.
+ * Returns TS_ERROR if hostname is not this machine.
+ */
+tsapi TSReturnCode TSHostnameIsSelf(const char *hostname);
+
+/*
+ * Gets the status of hostname in the outparam status, and the status reason in the outparam reason.
+ * The reason is a logical-and combination of the reasons in TSHostStatusReason.
Review comment:
This was moved from core. Fixed
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r499090278
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash.cc
##########
@@ -0,0 +1,483 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+// hash_key strings.
+constexpr std::string_view hash_key_url = "url";
+constexpr std::string_view hash_key_hostname = "hostname";
+constexpr std::string_view hash_key_path = "path";
+constexpr std::string_view hash_key_path_query = "path+query";
+constexpr std::string_view hash_key_path_fragment = "path+fragment";
+constexpr std::string_view hash_key_cache = "cache_key";
+
+static HostRecord *
+chash_lookup(std::shared_ptr<ATSConsistentHash> ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
+ ATSHash64Sip24 *hash, bool *hash_init, bool *mapWrapped, uint64_t sm_id)
+{
+ HostRecord *host_rec = nullptr;
+
+ if (*hash_init == false) {
+ host_rec = static_cast<HostRecord *>(ring->lookup_by_hashval(hash_key, iter, wrapped));
+ *hash_init = true;
+ } else {
+ host_rec = static_cast<HostRecord *>(ring->lookup(nullptr, iter, wrapped, hash));
+ }
+ bool wrap_around = *wrapped;
+ *wrapped = (*mapWrapped && *wrapped) ? true : false;
+ if (!*mapWrapped && wrap_around) {
+ *mapWrapped = true;
+ }
+
+ return host_rec;
+}
+
+NextHopConsistentHash::NextHopConsistentHash(const std::string_view name) : NextHopSelectionStrategy(name) {}
+
+NextHopConsistentHash::~NextHopConsistentHash()
+{
+ NH_Debug(NH_DEBUG_TAG, "destructor called for strategy named: %s", strategy_name.c_str());
+}
+
+bool
+NextHopConsistentHash::Init(const YAML::Node &n)
+{
+ ATSHash64Sip24 hash;
+
+ try {
+ if (n["hash_key"]) {
+ auto hash_key_val = n["hash_key"].Scalar();
+ if (hash_key_val == hash_key_url) {
+ hash_key = NH_URL_HASH_KEY;
+ } else if (hash_key_val == hash_key_hostname) {
+ hash_key = NH_HOSTNAME_HASH_KEY;
+ } else if (hash_key_val == hash_key_path) {
+ hash_key = NH_PATH_HASH_KEY;
+ } else if (hash_key_val == hash_key_path_query) {
+ hash_key = NH_PATH_QUERY_HASH_KEY;
+ } else if (hash_key_val == hash_key_path_fragment) {
+ hash_key = NH_PATH_FRAGMENT_HASH_KEY;
+ } else if (hash_key_val == hash_key_cache) {
+ hash_key = NH_CACHE_HASH_KEY;
+ } else {
+ hash_key = NH_PATH_HASH_KEY;
+ NH_Note("Invalid 'hash_key' value, '%s', for the strategy named '%s', using default '%s'.", hash_key_val.c_str(),
+ strategy_name.c_str(), hash_key_path.data());
+ }
+ }
+ } catch (std::exception &ex) {
+ NH_Note("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(), ex.what());
+ return false;
+ }
+
+ bool result = NextHopSelectionStrategy::Init(n);
+ if (!result) {
+ return false;
+ }
+
+ // load up the hash rings.
+ for (uint32_t i = 0; i < groups; i++) {
+ std::shared_ptr<ATSConsistentHash> hash_ring = std::make_shared<ATSConsistentHash>();
+ for (uint32_t j = 0; j < host_groups[i].size(); j++) {
+ // ATSConsistentHash needs the raw pointer.
+ HostRecord *p = host_groups[i][j].get();
+ // need to copy the 'hash_string' or 'hostname' cstring to 'name' for insertion into ATSConsistentHash.
+ if (!p->hash_string.empty()) {
+ p->name = const_cast<char *>(p->hash_string.c_str());
+ } else {
+ p->name = const_cast<char *>(p->hostname.c_str());
+ }
+ p->group_index = host_groups[i][j]->group_index;
+ p->host_index = host_groups[i][j]->host_index;
+ hash_ring->insert(p, p->weight, &hash);
+ NH_Debug(NH_DEBUG_TAG, "Loading hash rings - ring: %d, host record: %d, name: %s, hostname: %s, stategy: %s", i, j, p->name,
+ p->hostname.c_str(), strategy_name.c_str());
+ }
+ hash.clear();
+ rings.push_back(std::move(hash_ring));
Review comment:
Don't `move` a `shared_ptr`.
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469252277
##########
File path: include/ts/nexthop.h
##########
@@ -30,20 +30,20 @@
#include <ts/apidefs.h>
-// plugin callback commands.
enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
+class TSNextHopSelectionStrategy
Review comment:
Does this make the plugin API require C++?
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469688579
##########
File path: include/ts/parentresult.h
##########
@@ -0,0 +1,48 @@
+/** @file
+
+ @section license License
+
+ 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.
+ */
+
+#pragma once
+
+#include "ts/apidefs.h"
+#include "tscore/ConsistentHash.h"
+
+// Needed by core. Disabling the warning for plugins
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-variable"
+static const char *ParentResultStr[] = {"PARENT_UNDEFINED", "PARENT_DIRECT", "PARENT_SPECIFIED", "PARENT_AGENT", "PARENT_FAIL"};
Review comment:
Done
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469583614
##########
File path: include/ts/nexthop.h
##########
@@ -30,20 +30,20 @@
#include <ts/apidefs.h>
-// plugin callback commands.
enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
+class TSNextHopSelectionStrategy
Review comment:
I can move this to `include/tscpp`. Is that ok?
Sort of. `TSNextHopSelectionStrategy` is used by the Remap hook `TSRemapInitStrategy`, not in InkApi.cc. A Remap plugin can still be C and just not implement that function.
Likewise, `TSHttpTxnParentResult(G|S)et` in InkApi.cc uses a `TSParentResult`, which has C++ stuff in it. I just exposed that struct to the API, I didn't create it. If someone doesn't need to call that function, if I move `parentresult.h` to `include/tscpp`, they can write a C plugin and just not call the functions that use it.
Is that ok? Or is there a better way?
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r506615209
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -9790,6 +9791,54 @@ TSHttpTxnRedoCacheLookup(TSHttpTxn txnp, const char *url, int length)
return TS_ERROR;
}
+TSReturnCode
+TSHostnameIsSelf(const char *hostname)
+{
+ const bool isSelf = Machine::instance()->is_self(hostname);
Review comment:
Changed
----------------------------------------------------------------
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] [trafficserver] rob05c commented on pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#issuecomment-779331849
Closing in favor of https://github.com/apache/trafficserver/pull/7467
----------------------------------------------------------------
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] [trafficserver] rob05c closed pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c closed pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469564569
##########
File path: include/ts/ts.h
##########
@@ -29,6 +29,7 @@
#pragma once
+#include <string.h>
Review comment:
It's not. Removed
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r500350942
##########
File path: proxy/http/remap/RemapPluginInfo.cc
##########
@@ -271,6 +273,31 @@ RemapPluginInfo::indicatePostReload(TSRemapReloadStatus reloadStatus)
resetPluginContext();
}
+/* Initialize strategy (optional). */
+std::shared_ptr<TSNextHopSelectionStrategy>
+RemapPluginInfo::initStrategy(void *ih)
+{
+ if (!init_strategy_cb) {
+ PluginDebug(_tag, "plugin '%s' has no init_strategy_cb, returning nullptr", _configPath.c_str());
+ return std::shared_ptr<TSNextHopSelectionStrategy>();
+ }
+ char tmpbuf[2048];
+ ink_zero(tmpbuf);
+ TSNextHopSelectionStrategy *strategy_raw = nullptr;
+ if (init_strategy_cb(strategy_raw, ih, tmpbuf, sizeof(tmpbuf) - 1) != TS_SUCCESS) {
Review comment:
How does `strategy_raw` become not `nullptr`?
----------------------------------------------------------------
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] [trafficserver] rob05c commented on pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#issuecomment-769441694
Changing back to a draft: I have an idea to get the best of both worlds, small core changes like this PR, but also powerful and with real continuations, hooks, etc.
I'm not 100% sure the idea will work. If it does, I'll close those. If not, I'll un-draft it.
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r507923097
##########
File path: proxy/http/remap/RemapPluginInfo.cc
##########
@@ -271,6 +273,31 @@ RemapPluginInfo::indicatePostReload(TSRemapReloadStatus reloadStatus)
resetPluginContext();
}
+/* Initialize strategy (optional). */
+std::shared_ptr<TSNextHopSelectionStrategy>
+RemapPluginInfo::initStrategy(void *ih)
+{
+ if (!init_strategy_cb) {
+ PluginDebug(_tag, "plugin '%s' has no init_strategy_cb, returning nullptr", _configPath.c_str());
+ return std::shared_ptr<TSNextHopSelectionStrategy>();
+ }
+ char tmpbuf[2048];
+ ink_zero(tmpbuf);
+ TSNextHopSelectionStrategy *strategy_raw = nullptr;
+ if (init_strategy_cb(strategy_raw, ih, tmpbuf, sizeof(tmpbuf) - 1) != TS_SUCCESS) {
Review comment:
`init_strategy_cb` is `TSRemapInitStrategy`, which takes a `TSNextHopSelectionStrategy *&`. A plugin sets that pointer-ref. https://github.com/apache/trafficserver/blob/00e236d94aa5577e8a1d0f94d18787bd2c7e85b3/plugins/experimental/nexthop_strategy_consistenthash/plugin_consistenthash.cc#L74
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469693966
##########
File path: include/ts/parentresult.h
##########
@@ -0,0 +1,48 @@
+/** @file
+
+ @section license License
+
+ 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.
+ */
+
+#pragma once
+
+#include "ts/apidefs.h"
+#include "tscore/ConsistentHash.h"
+
+// Needed by core. Disabling the warning for plugins
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-variable"
+static const char *ParentResultStr[] = {"PARENT_UNDEFINED", "PARENT_DIRECT", "PARENT_SPECIFIED", "PARENT_AGENT", "PARENT_FAIL"};
+#pragma GCC diagnostic pop
+
+struct TSParentResult {
+ const char *hostname;
+ int port;
+ bool retry;
+ TSParentResultType result;
+ bool chash_init[TS_MAX_GROUP_RINGS] = {false};
Review comment:
Yeah, the object is C++, unfortunately.
This is an API version of a core struct that already exists. If it's changed here, moving the data between the API and core will have to translate between them, and will probably be less efficient.
Alternatively, I could change it in core. It looks like that wouldn't be too invasive.
I haven't benchmarked, but it was my understanding that std::bitset is optimized for space, and is often less performant than `bool[]` or bitmasking. Notwithstanding, I'd still prefer to do a bitmask, because the struct only has one C++ member, and I'd like to move it in the direction of C for the sake of the API.
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r507920903
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -9790,6 +9791,54 @@ TSHttpTxnRedoCacheLookup(TSHttpTxn txnp, const char *url, int length)
return TS_ERROR;
}
+TSReturnCode
+TSHostnameIsSelf(const char *hostname)
+{
+ const bool isSelf = Machine::instance()->is_self(hostname);
+ if (isSelf) {
+ return TS_SUCCESS;
+ }
+ return TS_ERROR;
+}
+
+TSReturnCode
+TSHostStatusGet(const char *hostname, TSHostStatus *status, unsigned int *reason)
Review comment:
Done
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r499090232
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash.cc
##########
@@ -0,0 +1,483 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+// hash_key strings.
+constexpr std::string_view hash_key_url = "url";
+constexpr std::string_view hash_key_hostname = "hostname";
+constexpr std::string_view hash_key_path = "path";
+constexpr std::string_view hash_key_path_query = "path+query";
+constexpr std::string_view hash_key_path_fragment = "path+fragment";
+constexpr std::string_view hash_key_cache = "cache_key";
+
+static HostRecord *
+chash_lookup(std::shared_ptr<ATSConsistentHash> ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
+ ATSHash64Sip24 *hash, bool *hash_init, bool *mapWrapped, uint64_t sm_id)
+{
+ HostRecord *host_rec = nullptr;
+
+ if (*hash_init == false) {
+ host_rec = static_cast<HostRecord *>(ring->lookup_by_hashval(hash_key, iter, wrapped));
+ *hash_init = true;
+ } else {
+ host_rec = static_cast<HostRecord *>(ring->lookup(nullptr, iter, wrapped, hash));
+ }
+ bool wrap_around = *wrapped;
+ *wrapped = (*mapWrapped && *wrapped) ? true : false;
+ if (!*mapWrapped && wrap_around) {
+ *mapWrapped = true;
+ }
+
+ return host_rec;
+}
+
+NextHopConsistentHash::NextHopConsistentHash(const std::string_view name) : NextHopSelectionStrategy(name) {}
+
+NextHopConsistentHash::~NextHopConsistentHash()
+{
+ NH_Debug(NH_DEBUG_TAG, "destructor called for strategy named: %s", strategy_name.c_str());
+}
+
+bool
+NextHopConsistentHash::Init(const YAML::Node &n)
+{
+ ATSHash64Sip24 hash;
+
+ try {
+ if (n["hash_key"]) {
+ auto hash_key_val = n["hash_key"].Scalar();
+ if (hash_key_val == hash_key_url) {
+ hash_key = NH_URL_HASH_KEY;
+ } else if (hash_key_val == hash_key_hostname) {
+ hash_key = NH_HOSTNAME_HASH_KEY;
+ } else if (hash_key_val == hash_key_path) {
+ hash_key = NH_PATH_HASH_KEY;
+ } else if (hash_key_val == hash_key_path_query) {
+ hash_key = NH_PATH_QUERY_HASH_KEY;
+ } else if (hash_key_val == hash_key_path_fragment) {
+ hash_key = NH_PATH_FRAGMENT_HASH_KEY;
+ } else if (hash_key_val == hash_key_cache) {
+ hash_key = NH_CACHE_HASH_KEY;
+ } else {
+ hash_key = NH_PATH_HASH_KEY;
+ NH_Note("Invalid 'hash_key' value, '%s', for the strategy named '%s', using default '%s'.", hash_key_val.c_str(),
+ strategy_name.c_str(), hash_key_path.data());
+ }
+ }
+ } catch (std::exception &ex) {
+ NH_Note("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(), ex.what());
+ return false;
+ }
+
+ bool result = NextHopSelectionStrategy::Init(n);
+ if (!result) {
+ return false;
+ }
+
+ // load up the hash rings.
+ for (uint32_t i = 0; i < groups; i++) {
+ std::shared_ptr<ATSConsistentHash> hash_ring = std::make_shared<ATSConsistentHash>();
+ for (uint32_t j = 0; j < host_groups[i].size(); j++) {
+ // ATSConsistentHash needs the raw pointer.
+ HostRecord *p = host_groups[i][j].get();
+ // need to copy the 'hash_string' or 'hostname' cstring to 'name' for insertion into ATSConsistentHash.
+ if (!p->hash_string.empty()) {
+ p->name = const_cast<char *>(p->hash_string.c_str());
Review comment:
Is `const_cast` really required here? It's a bit risky.
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469583614
##########
File path: include/ts/nexthop.h
##########
@@ -30,20 +30,20 @@
#include <ts/apidefs.h>
-// plugin callback commands.
enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
+class TSNextHopSelectionStrategy
Review comment:
I can move this to `include/tscpp`. Is that ok?
Sort of. `TSNextHopSelectionStrategy` is used by the Remap hook `TSRemapInitStrategy`, not in InkApi.cc. A Remap plugin can still be C and just not implement that function.
Likewise, `TSHttpTxnParentResult(G|S)et` in InkApi.cc uses a `TSParentResult`, which has C++ stuff in it. I just exposed that struct to the API, I didn't create it. If someone doesn't need to call that function, if I move `parentresult.h` to `include/tscpp`, someone can write a C plugin and just not call the functions that use it.
Is that ok? Or is there a better way?
----------------------------------------------------------------
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] [trafficserver] brbzull0 commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r518134060
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash_config.cc
##########
@@ -0,0 +1,207 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+void loadConfigFile(const std::string fileName, std::stringstream &doc, std::unordered_set<std::string> &include_once);
+
+// createStrategy creates and initializes a Consistent Hash strategy from the given YAML node.
+// Caller takes ownership of the returned pointer, and must call delete on it.
+TSNextHopSelectionStrategy *
+createStrategy(const std::string &name, const YAML::Node &node)
+{
+ NextHopConsistentHash *st = new NextHopConsistentHash(name);
+ if (!st->Init(node)) {
+ return nullptr;
+ }
+ return st;
+}
+
+// createStrategyFromFile creates a Consistent Hash strategy from the given config file.
+// Caller takes ownership of the returned pointer, and must call delete on it.
+TSNextHopSelectionStrategy *
+createStrategyFromFile(const char *file, const char *strategyName)
+{
+ NH_Debug(NH_DEBUG_TAG, "plugin createStrategyFromFile file '%s' strategy '%s'", file, strategyName);
+
+ YAML::Node config;
+ YAML::Node strategies;
+ std::stringstream doc;
+ std::unordered_set<std::string> include_once;
+ std::string_view fn = file;
+
+ // strategy policy
+ constexpr std::string_view consistent_hash = "consistent_hash";
+
+ const char *basename = fn.substr(fn.find_last_of('/') + 1).data();
+
+ try {
+ NH_Note("%s loading ...", basename);
+ loadConfigFile(fn.data(), doc, include_once);
+
+ config = YAML::Load(doc);
+ if (config.IsNull()) {
+ NH_Note("No NextHop strategy configs were loaded.");
+ return nullptr;
+ }
+
+ strategies = config["strategies"];
+ if (strategies.Type() != YAML::NodeType::Sequence) {
+ NH_Error("malformed %s file, expected a 'strategies' sequence", basename);
+ return nullptr;
+ }
+
+ for (unsigned int i = 0; i < strategies.size(); ++i) {
+ YAML::Node strategy = strategies[i];
Review comment:
Have you considered following [this](https://github.com/jbeder/yaml-cpp/wiki/Tutorial#converting-tofrom-native-data-types) custom Data types like `TSNextHopSelectionStrategy`?
----------------------------------------------------------------
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] [trafficserver] a-canary commented on pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
a-canary commented on pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#issuecomment-705798874
-0, AFAIK the general plugin design is to use event handlers. Which I realized will take a lot more work but also simplify the API and the HttpSM quite a bit in the end. I hope we can move towards that once Susan gets the ConnectionSM refactored.
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r499075587
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash.cc
##########
@@ -0,0 +1,483 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+// hash_key strings.
+constexpr std::string_view hash_key_url = "url";
+constexpr std::string_view hash_key_hostname = "hostname";
+constexpr std::string_view hash_key_path = "path";
+constexpr std::string_view hash_key_path_query = "path+query";
+constexpr std::string_view hash_key_path_fragment = "path+fragment";
+constexpr std::string_view hash_key_cache = "cache_key";
+
+static HostRecord *
+chash_lookup(std::shared_ptr<ATSConsistentHash> ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
Review comment:
This is more than enough state to justify an iterator.
----------------------------------------------------------------
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] [trafficserver] randall commented on pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#issuecomment-662609836
[approve ci freebsd]
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r500364895
##########
File path: proxy/http/remap/PluginFactory.h
##########
@@ -54,6 +54,10 @@ class RemapPluginInst
TSRemapStatus doRemap(TSHttpTxn rh, TSRemapRequestInfo *rri);
void osResponse(TSHttpTxn rh, int os_response_type);
+ /* gets the Strategy if the plugin has a TSRemapInitStrategy function. Returns a nullptr if not. */
+ std::shared_ptr<TSNextHopSelectionStrategy> getStrategy();
+ std::string name() const;
Review comment:
Noooooooooo! Don't return a `std::string`.
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r503957739
##########
File path: proxy/http/remap/PluginFactory.cc
##########
@@ -76,6 +76,18 @@ RemapPluginInst::doRemap(TSHttpTxn rh, TSRemapRequestInfo *rri)
return _plugin.doRemap(_instance, rh, rri);
}
+std::shared_ptr<TSNextHopSelectionStrategy>
+RemapPluginInst::getStrategy()
Review comment:
There seems a bit of a name semantic mismatch - the outer is "get" but the internal is "init". Does this initialize or just get?
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r506606221
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash.cc
##########
@@ -0,0 +1,483 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+// hash_key strings.
+constexpr std::string_view hash_key_url = "url";
+constexpr std::string_view hash_key_hostname = "hostname";
+constexpr std::string_view hash_key_path = "path";
+constexpr std::string_view hash_key_path_query = "path+query";
+constexpr std::string_view hash_key_path_fragment = "path+fragment";
+constexpr std::string_view hash_key_cache = "cache_key";
+
+static HostRecord *
+chash_lookup(std::shared_ptr<ATSConsistentHash> ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
+ ATSHash64Sip24 *hash, bool *hash_init, bool *mapWrapped, uint64_t sm_id)
+{
+ HostRecord *host_rec = nullptr;
+
+ if (*hash_init == false) {
+ host_rec = static_cast<HostRecord *>(ring->lookup_by_hashval(hash_key, iter, wrapped));
+ *hash_init = true;
+ } else {
+ host_rec = static_cast<HostRecord *>(ring->lookup(nullptr, iter, wrapped, hash));
+ }
+ bool wrap_around = *wrapped;
+ *wrapped = (*mapWrapped && *wrapped) ? true : false;
Review comment:
Copied from core. Fixed.
For the record, I would never write `? true : false` 🤦
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469253060
##########
File path: include/ts/parentresult.h
##########
@@ -0,0 +1,48 @@
+/** @file
+
+ @section license License
+
+ 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.
+ */
+
+#pragma once
+
+#include "ts/apidefs.h"
+#include "tscore/ConsistentHash.h"
+
+// Needed by core. Disabling the warning for plugins
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-variable"
+static const char *ParentResultStr[] = {"PARENT_UNDEFINED", "PARENT_DIRECT", "PARENT_SPECIFIED", "PARENT_AGENT", "PARENT_FAIL"};
Review comment:
This should be defined in a ".cc" file and only the extern should be here. That should (1) avoid having multiple copies of this table and (2) avoid the warning.
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r503919553
##########
File path: include/tscpp/api/parentresult.h
##########
@@ -0,0 +1,44 @@
+/** @file
+
+ @section license License
+
+ 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.
+ */
+
+#pragma once
+
+#include "ts/apidefs.h"
+#include "tscore/ConsistentHash.h"
+
+extern const char *ParentResultStr[];
+
+struct TSParentResult {
+ const char *hostname;
+ int port;
+ bool retry;
+ TSParentResultType result;
+ bool chash_init[TS_MAX_GROUP_RINGS] = {false};
+ TSHostStatus first_choice_status = TSHostStatus::TS_HOST_STATUS_INIT;
+ int line_number;
+ uint32_t last_parent;
+ uint32_t start_parent;
+ uint32_t last_group;
+ bool wrap_around;
+ bool mapWrapped[2];
+ int last_lookup;
+ ATSConsistentHashIter chashIter[TS_MAX_GROUP_RINGS];
Review comment:
Are the hash values and iterators needed in this result structure? Is this intended as a state object and not just a result?
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r506604573
##########
File path: include/tscpp/api/parentresult.h
##########
@@ -0,0 +1,44 @@
+/** @file
+
+ @section license License
+
+ 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.
+ */
+
+#pragma once
+
+#include "ts/apidefs.h"
+#include "tscore/ConsistentHash.h"
+
+extern const char *ParentResultStr[];
+
+struct TSParentResult {
+ const char *hostname;
+ int port;
Review comment:
Copied from core. Fixed
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r507996700
##########
File path: include/tscpp/api/nexthop.h
##########
@@ -30,20 +30,20 @@
#include <ts/apidefs.h>
-// plugin callback commands.
enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
+class TSNextHopSelectionStrategy
+{
+public:
+ TSNextHopSelectionStrategy(){};
+ virtual ~TSNextHopSelectionStrategy(){};
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
+ virtual void findNextHop(TSHttpTxn txnp, time_t now = 0) = 0;
+ virtual void markNextHop(TSHttpTxn txnp, const char *hostname, const int port, const NHCmd status, const time_t now = 0) = 0;
+ virtual bool nextHopExists(TSHttpTxn txnp) = 0;
Review comment:
`findNextHop` modifies the state to be the next parent; `nextHopExists` returns without modifying state.
I'd be a big +1 on making `findNextHop` return and be a pure function. But that'd require refactoring how Strategies work and are called in core. Which I'm guessing itself is a reflection of how ParentSelection always did it.
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r506609167
##########
File path: proxy/ParentSelection.cc
##########
@@ -1869,23 +1871,65 @@ void
show_result(ParentResult *p)
{
switch (p->result) {
- case PARENT_UNDEFINED:
- printf("result is PARENT_UNDEFINED\n");
+ case TS_PARENT_UNDEFINED:
+ printf("result is TS_PARENT_UNDEFINED\n");
break;
- case PARENT_DIRECT:
- printf("result is PARENT_DIRECT\n");
+ case TS_PARENT_DIRECT:
+ printf("result is TS_PARENT_DIRECT\n");
break;
- case PARENT_SPECIFIED:
- printf("result is PARENT_SPECIFIED\n");
+ case TS_PARENT_SPECIFIED:
+ printf("result is TS_PARENT_SPECIFIED\n");
printf("hostname is %s\n", p->hostname);
printf("port is %d\n", p->port);
break;
- case PARENT_FAIL:
- printf("result is PARENT_FAIL\n");
+ case TS_PARENT_FAIL:
+ printf("result is TS_PARENT_FAIL\n");
break;
default:
// Handled here:
// PARENT_AGENT
break;
}
}
+
+void
+ParentResult::copyFrom(TSParentResult *r)
+{
+ this->hostname = r->hostname;
+ this->port = r->port;
+ this->retry = r->retry;
+ this->result = r->result;
+ memcpy(this->chash_init, r->chash_init, TS_MAX_GROUP_RINGS * sizeof(bool));
+ this->first_choice_status = r->first_choice_status;
+ this->line_number = r->line_number;
+ this->last_parent = r->last_parent;
+ this->start_parent = r->start_parent;
+ this->last_group = r->last_group;
+ this->wrap_around = r->wrap_around;
+ memcpy(this->mapWrapped, r->mapWrapped, 2 * sizeof(bool));
+ this->last_lookup = r->last_lookup;
+ for (int i = 0; i < TS_MAX_GROUP_RINGS; ++i) {
+ this->chashIter[i] = r->chashIter[i];
+ }
+}
+
+void
+ParentResult::copyTo(TSParentResult *r)
Review comment:
Eh, I lean against obfuscating nontrivial copies, and these funcs are just complex enough to be nontrivial. I can change it if you want / if overriding assignment is the idiom in ATS
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r469254743
##########
File path: include/ts/parentresult.h
##########
@@ -0,0 +1,48 @@
+/** @file
+
+ @section license License
+
+ 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.
+ */
+
+#pragma once
+
+#include "ts/apidefs.h"
+#include "tscore/ConsistentHash.h"
+
+// Needed by core. Disabling the warning for plugins
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-variable"
+static const char *ParentResultStr[] = {"PARENT_UNDEFINED", "PARENT_DIRECT", "PARENT_SPECIFIED", "PARENT_AGENT", "PARENT_FAIL"};
+#pragma GCC diagnostic pop
+
+struct TSParentResult {
+ const char *hostname;
+ int port;
+ bool retry;
+ TSParentResultType result;
+ bool chash_init[TS_MAX_GROUP_RINGS] = {false};
Review comment:
Hmmm, is this C++? If so, `std::bitset` is a better choice here, otherwise I'd look at making this a bit mask object.
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r508004669
##########
File path: include/tscpp/api/parentresult.h
##########
@@ -0,0 +1,44 @@
+/** @file
+
+ @section license License
+
+ 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.
+ */
+
+#pragma once
+
+#include "ts/apidefs.h"
+#include "tscore/ConsistentHash.h"
+
+extern const char *ParentResultStr[];
+
+struct TSParentResult {
+ const char *hostname;
+ int port;
+ bool retry;
+ TSParentResultType result;
+ bool chash_init[TS_MAX_GROUP_RINGS] = {false};
+ TSHostStatus first_choice_status = TSHostStatus::TS_HOST_STATUS_INIT;
+ int line_number;
+ uint32_t last_parent;
+ uint32_t start_parent;
+ uint32_t last_group;
+ bool wrap_around;
+ bool mapWrapped[2];
+ int last_lookup;
+ ATSConsistentHashIter chashIter[TS_MAX_GROUP_RINGS];
Review comment:
Yeah, it's a state object. Used by both ParentSelection and Strategies in core (or rather, copied to an identical structure in core).
I.e.
https://github.com/apache/trafficserver/blob/786463/proxy/ParentConsistentHash.cc#L110
https://github.com/apache/trafficserver/blob/786463/proxy/http/remap/NextHopConsistentHash.cc#L335
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r503905766
##########
File path: include/tscpp/api/nexthop.h
##########
@@ -30,20 +30,20 @@
#include <ts/apidefs.h>
-// plugin callback commands.
enum NHCmd { NH_MARK_UP, NH_MARK_DOWN };
-struct NHHealthStatus {
- virtual bool isNextHopAvailable(TSHttpTxn txn, const char *hostname, const int port, void *ih = nullptr) = 0;
- virtual void markNextHop(TSHttpTxn txn, const char *hostname, const int port, const NHCmd status, void *ih = nullptr,
- const time_t now = 0) = 0;
- virtual ~NHHealthStatus() {}
-};
+class TSNextHopSelectionStrategy
+{
+public:
+ TSNextHopSelectionStrategy(){};
+ virtual ~TSNextHopSelectionStrategy(){};
-struct NHPluginStrategy {
- virtual void findNextHop(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual bool nextHopExists(TSHttpTxn txnp, void *ih = nullptr) = 0;
- virtual ~NHPluginStrategy() {}
+ virtual void findNextHop(TSHttpTxn txnp, time_t now = 0) = 0;
+ virtual void markNextHop(TSHttpTxn txnp, const char *hostname, const int port, const NHCmd status, const time_t now = 0) = 0;
+ virtual bool nextHopExists(TSHttpTxn txnp) = 0;
Review comment:
Is this really useful? Why not have `findNextHop` return `bool`?
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r499074185
##########
File path: include/tscpp/api/parentresult.h
##########
@@ -0,0 +1,44 @@
+/** @file
+
+ @section license License
+
+ 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.
+ */
+
+#pragma once
+
+#include "ts/apidefs.h"
+#include "tscore/ConsistentHash.h"
+
+extern const char *ParentResultStr[];
+
+struct TSParentResult {
+ const char *hostname;
+ int port;
Review comment:
`in_port_t`
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r507990059
##########
File path: proxy/http/remap/PluginFactory.h
##########
@@ -54,6 +54,10 @@ class RemapPluginInst
TSRemapStatus doRemap(TSHttpTxn rh, TSRemapRequestInfo *rri);
void osResponse(TSHttpTxn rh, int os_response_type);
+ /* gets the Strategy if the plugin has a TSRemapInitStrategy function. Returns a nullptr if not. */
+ std::shared_ptr<TSNextHopSelectionStrategy> getStrategy();
+ std::string name() const;
Review comment:
Done
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r507990059
##########
File path: proxy/http/remap/PluginFactory.h
##########
@@ -54,6 +54,10 @@ class RemapPluginInst
TSRemapStatus doRemap(TSHttpTxn rh, TSRemapRequestInfo *rri);
void osResponse(TSHttpTxn rh, int os_response_type);
+ /* gets the Strategy if the plugin has a TSRemapInitStrategy function. Returns a nullptr if not. */
+ std::shared_ptr<TSNextHopSelectionStrategy> getStrategy();
+ std::string name() const;
Review comment:
Changed to string_view, with a comment about the lifetime
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r508026325
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash.cc
##########
@@ -0,0 +1,483 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+// hash_key strings.
+constexpr std::string_view hash_key_url = "url";
+constexpr std::string_view hash_key_hostname = "hostname";
+constexpr std::string_view hash_key_path = "path";
+constexpr std::string_view hash_key_path_query = "path+query";
+constexpr std::string_view hash_key_path_fragment = "path+fragment";
+constexpr std::string_view hash_key_cache = "cache_key";
+
+static HostRecord *
+chash_lookup(std::shared_ptr<ATSConsistentHash> ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
+ ATSHash64Sip24 *hash, bool *hash_init, bool *mapWrapped, uint64_t sm_id)
+{
+ HostRecord *host_rec = nullptr;
+
+ if (*hash_init == false) {
+ host_rec = static_cast<HostRecord *>(ring->lookup_by_hashval(hash_key, iter, wrapped));
+ *hash_init = true;
+ } else {
+ host_rec = static_cast<HostRecord *>(ring->lookup(nullptr, iter, wrapped, hash));
+ }
+ bool wrap_around = *wrapped;
+ *wrapped = (*mapWrapped && *wrapped) ? true : false;
+ if (!*mapWrapped && wrap_around) {
+ *mapWrapped = true;
+ }
+
+ return host_rec;
+}
+
+NextHopConsistentHash::NextHopConsistentHash(const std::string_view name) : NextHopSelectionStrategy(name) {}
+
+NextHopConsistentHash::~NextHopConsistentHash()
+{
+ NH_Debug(NH_DEBUG_TAG, "destructor called for strategy named: %s", strategy_name.c_str());
+}
+
+bool
+NextHopConsistentHash::Init(const YAML::Node &n)
+{
+ ATSHash64Sip24 hash;
+
+ try {
+ if (n["hash_key"]) {
+ auto hash_key_val = n["hash_key"].Scalar();
+ if (hash_key_val == hash_key_url) {
+ hash_key = NH_URL_HASH_KEY;
+ } else if (hash_key_val == hash_key_hostname) {
+ hash_key = NH_HOSTNAME_HASH_KEY;
+ } else if (hash_key_val == hash_key_path) {
+ hash_key = NH_PATH_HASH_KEY;
+ } else if (hash_key_val == hash_key_path_query) {
+ hash_key = NH_PATH_QUERY_HASH_KEY;
+ } else if (hash_key_val == hash_key_path_fragment) {
+ hash_key = NH_PATH_FRAGMENT_HASH_KEY;
+ } else if (hash_key_val == hash_key_cache) {
+ hash_key = NH_CACHE_HASH_KEY;
+ } else {
+ hash_key = NH_PATH_HASH_KEY;
+ NH_Note("Invalid 'hash_key' value, '%s', for the strategy named '%s', using default '%s'.", hash_key_val.c_str(),
+ strategy_name.c_str(), hash_key_path.data());
+ }
+ }
+ } catch (std::exception &ex) {
+ NH_Note("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(), ex.what());
+ return false;
+ }
+
+ bool result = NextHopSelectionStrategy::Init(n);
+ if (!result) {
+ return false;
+ }
+
+ // load up the hash rings.
+ for (uint32_t i = 0; i < groups; i++) {
+ std::shared_ptr<ATSConsistentHash> hash_ring = std::make_shared<ATSConsistentHash>();
+ for (uint32_t j = 0; j < host_groups[i].size(); j++) {
+ // ATSConsistentHash needs the raw pointer.
+ HostRecord *p = host_groups[i][j].get();
+ // need to copy the 'hash_string' or 'hostname' cstring to 'name' for insertion into ATSConsistentHash.
+ if (!p->hash_string.empty()) {
+ p->name = const_cast<char *>(p->hash_string.c_str());
+ } else {
+ p->name = const_cast<char *>(p->hostname.c_str());
+ }
+ p->group_index = host_groups[i][j]->group_index;
+ p->host_index = host_groups[i][j]->host_index;
+ hash_ring->insert(p, p->weight, &hash);
+ NH_Debug(NH_DEBUG_TAG, "Loading hash rings - ring: %d, host record: %d, name: %s, hostname: %s, stategy: %s", i, j, p->name,
+ p->hostname.c_str(), strategy_name.c_str());
+ }
+ hash.clear();
+ rings.push_back(std::move(hash_ring));
Review comment:
Changed
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r499073793
##########
File path: include/ts/ts.h
##########
@@ -2573,6 +2573,38 @@ tsapi TSIOBufferReader TSHttpTxnPostBufferReaderGet(TSHttpTxn txnp);
*/
tsapi TSReturnCode TSHttpTxnServerPush(TSHttpTxn txnp, const char *url, int url_len);
+/*
+ * Returns TS_SUCCESS if hostname is this machine, as used for parent and remap self-detection.
+ * Returns TS_ERROR if hostname is not this machine.
+ */
+tsapi TSReturnCode TSHostnameIsSelf(const char *hostname);
+
+/*
+ * Gets the status of hostname in the outparam status, and the status reason in the outparam reason.
+ * The reason is a logical-and combination of the reasons in TSHostStatusReason.
Review comment:
Logical or, maybe? Logical and would always be zero.
----------------------------------------------------------------
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] [trafficserver] brbzull0 commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r518134060
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash_config.cc
##########
@@ -0,0 +1,207 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+void loadConfigFile(const std::string fileName, std::stringstream &doc, std::unordered_set<std::string> &include_once);
+
+// createStrategy creates and initializes a Consistent Hash strategy from the given YAML node.
+// Caller takes ownership of the returned pointer, and must call delete on it.
+TSNextHopSelectionStrategy *
+createStrategy(const std::string &name, const YAML::Node &node)
+{
+ NextHopConsistentHash *st = new NextHopConsistentHash(name);
+ if (!st->Init(node)) {
+ return nullptr;
+ }
+ return st;
+}
+
+// createStrategyFromFile creates a Consistent Hash strategy from the given config file.
+// Caller takes ownership of the returned pointer, and must call delete on it.
+TSNextHopSelectionStrategy *
+createStrategyFromFile(const char *file, const char *strategyName)
+{
+ NH_Debug(NH_DEBUG_TAG, "plugin createStrategyFromFile file '%s' strategy '%s'", file, strategyName);
+
+ YAML::Node config;
+ YAML::Node strategies;
+ std::stringstream doc;
+ std::unordered_set<std::string> include_once;
+ std::string_view fn = file;
+
+ // strategy policy
+ constexpr std::string_view consistent_hash = "consistent_hash";
+
+ const char *basename = fn.substr(fn.find_last_of('/') + 1).data();
+
+ try {
+ NH_Note("%s loading ...", basename);
+ loadConfigFile(fn.data(), doc, include_once);
+
+ config = YAML::Load(doc);
+ if (config.IsNull()) {
+ NH_Note("No NextHop strategy configs were loaded.");
+ return nullptr;
+ }
+
+ strategies = config["strategies"];
+ if (strategies.Type() != YAML::NodeType::Sequence) {
+ NH_Error("malformed %s file, expected a 'strategies' sequence", basename);
+ return nullptr;
+ }
+
+ for (unsigned int i = 0; i < strategies.size(); ++i) {
+ YAML::Node strategy = strategies[i];
Review comment:
Have you considered following [this](https://github.com/jbeder/yaml-cpp/wiki/Tutorial#converting-tofrom-native-data-types) for custom Data Types like `TSNextHopSelectionStrategy`?
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r500321510
##########
File path: src/traffic_server/InkAPI.cc
##########
@@ -9790,6 +9791,54 @@ TSHttpTxnRedoCacheLookup(TSHttpTxn txnp, const char *url, int length)
return TS_ERROR;
}
+TSReturnCode
+TSHostnameIsSelf(const char *hostname)
+{
+ const bool isSelf = Machine::instance()->is_self(hostname);
Review comment:
```
return Machine::instance()->is_self(hostname) ? TS_SUCCESS : TS_ERROR;
```
?
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r508049200
##########
File path: proxy/http/remap/PluginFactory.cc
##########
@@ -76,6 +76,18 @@ RemapPluginInst::doRemap(TSHttpTxn rh, TSRemapRequestInfo *rri)
return _plugin.doRemap(_instance, rh, rri);
}
+std::shared_ptr<TSNextHopSelectionStrategy>
+RemapPluginInst::getStrategy()
Review comment:
Done
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r506607736
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash.cc
##########
@@ -0,0 +1,483 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+// hash_key strings.
+constexpr std::string_view hash_key_url = "url";
+constexpr std::string_view hash_key_hostname = "hostname";
+constexpr std::string_view hash_key_path = "path";
+constexpr std::string_view hash_key_path_query = "path+query";
+constexpr std::string_view hash_key_path_fragment = "path+fragment";
+constexpr std::string_view hash_key_cache = "cache_key";
+
+static HostRecord *
+chash_lookup(std::shared_ptr<ATSConsistentHash> ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
+ ATSHash64Sip24 *hash, bool *hash_init, bool *mapWrapped, uint64_t sm_id)
+{
+ HostRecord *host_rec = nullptr;
+
+ if (*hash_init == false) {
+ host_rec = static_cast<HostRecord *>(ring->lookup_by_hashval(hash_key, iter, wrapped));
+ *hash_init = true;
+ } else {
+ host_rec = static_cast<HostRecord *>(ring->lookup(nullptr, iter, wrapped, hash));
+ }
+ bool wrap_around = *wrapped;
+ *wrapped = (*mapWrapped && *wrapped) ? true : false;
+ if (!*mapWrapped && wrap_around) {
+ *mapWrapped = true;
+ }
+
+ return host_rec;
+}
+
+NextHopConsistentHash::NextHopConsistentHash(const std::string_view name) : NextHopSelectionStrategy(name) {}
+
+NextHopConsistentHash::~NextHopConsistentHash()
+{
+ NH_Debug(NH_DEBUG_TAG, "destructor called for strategy named: %s", strategy_name.c_str());
+}
+
+bool
+NextHopConsistentHash::Init(const YAML::Node &n)
+{
+ ATSHash64Sip24 hash;
+
+ try {
+ if (n["hash_key"]) {
+ auto hash_key_val = n["hash_key"].Scalar();
+ if (hash_key_val == hash_key_url) {
+ hash_key = NH_URL_HASH_KEY;
+ } else if (hash_key_val == hash_key_hostname) {
+ hash_key = NH_HOSTNAME_HASH_KEY;
+ } else if (hash_key_val == hash_key_path) {
+ hash_key = NH_PATH_HASH_KEY;
+ } else if (hash_key_val == hash_key_path_query) {
+ hash_key = NH_PATH_QUERY_HASH_KEY;
+ } else if (hash_key_val == hash_key_path_fragment) {
+ hash_key = NH_PATH_FRAGMENT_HASH_KEY;
+ } else if (hash_key_val == hash_key_cache) {
+ hash_key = NH_CACHE_HASH_KEY;
+ } else {
+ hash_key = NH_PATH_HASH_KEY;
+ NH_Note("Invalid 'hash_key' value, '%s', for the strategy named '%s', using default '%s'.", hash_key_val.c_str(),
+ strategy_name.c_str(), hash_key_path.data());
+ }
+ }
+ } catch (std::exception &ex) {
+ NH_Note("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(), ex.what());
+ return false;
+ }
+
+ bool result = NextHopSelectionStrategy::Init(n);
+ if (!result) {
+ return false;
+ }
+
+ // load up the hash rings.
+ for (uint32_t i = 0; i < groups; i++) {
+ std::shared_ptr<ATSConsistentHash> hash_ring = std::make_shared<ATSConsistentHash>();
+ for (uint32_t j = 0; j < host_groups[i].size(); j++) {
+ // ATSConsistentHash needs the raw pointer.
+ HostRecord *p = host_groups[i][j].get();
+ // need to copy the 'hash_string' or 'hostname' cstring to 'name' for insertion into ATSConsistentHash.
+ if (!p->hash_string.empty()) {
+ p->name = const_cast<char *>(p->hash_string.c_str());
Review comment:
Also copied from core. Yeah, because `HostRecord.name` is not const. I was told it couldn't be for reasons, and that it would never be changed. Likewise, I was assured hash_string would always outlive it, making returning the c_str safe.
I can try to verify those things, but it would take some time. FWIW I certainly don't like it either, but didn't see an obvious alternative.
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r499077003
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash.cc
##########
@@ -0,0 +1,483 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+// hash_key strings.
+constexpr std::string_view hash_key_url = "url";
+constexpr std::string_view hash_key_hostname = "hostname";
+constexpr std::string_view hash_key_path = "path";
+constexpr std::string_view hash_key_path_query = "path+query";
+constexpr std::string_view hash_key_path_fragment = "path+fragment";
+constexpr std::string_view hash_key_cache = "cache_key";
+
+static HostRecord *
+chash_lookup(std::shared_ptr<ATSConsistentHash> ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
+ ATSHash64Sip24 *hash, bool *hash_init, bool *mapWrapped, uint64_t sm_id)
+{
+ HostRecord *host_rec = nullptr;
+
+ if (*hash_init == false) {
+ host_rec = static_cast<HostRecord *>(ring->lookup_by_hashval(hash_key, iter, wrapped));
+ *hash_init = true;
+ } else {
+ host_rec = static_cast<HostRecord *>(ring->lookup(nullptr, iter, wrapped, hash));
+ }
+ bool wrap_around = *wrapped;
+ *wrapped = (*mapWrapped && *wrapped) ? true : false;
Review comment:
Why the ternary? Just `*wrapped = (*mapWrapped && *wrapped) ;`
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r508005260
##########
File path: proxy/http/remap/PluginFactory.cc
##########
@@ -76,6 +76,18 @@ RemapPluginInst::doRemap(TSHttpTxn rh, TSRemapRequestInfo *rri)
return _plugin.doRemap(_instance, rh, rri);
}
+std::shared_ptr<TSNextHopSelectionStrategy>
+RemapPluginInst::getStrategy()
Review comment:
This gets the strategy that the func it calls initializes. It's conceptually debatable, I don't object to renaming it
----------------------------------------------------------------
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] [trafficserver] randall removed a comment on pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
randall removed a comment on pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#issuecomment-662609836
[approve ci freebsd]
----------------------------------------------------------------
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] [trafficserver] rob05c commented on a change in pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#discussion_r508619602
##########
File path: plugins/experimental/nexthop_strategy_consistenthash/consistenthash.cc
##########
@@ -0,0 +1,483 @@
+/*
+ * 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.
+ */
+
+#include "strategy.h"
+#include "consistenthash.h"
+#include "util.h"
+
+#include <cinttypes>
+#include <string>
+#include <algorithm>
+#include <mutex>
+#include <unordered_map>
+#include <unordered_set>
+#include <fstream>
+#include <cstring>
+
+#include <sys/stat.h>
+#include <dirent.h>
+
+#include <yaml-cpp/yaml.h>
+
+#include "tscore/HashSip.h"
+#include "tscore/ConsistentHash.h"
+#include "tscore/ink_assert.h"
+#include "ts/ts.h"
+#include "ts/remap.h"
+#include "tscpp/api/nexthop.h"
+#include "tscpp/api/parentresult.h"
+
+// hash_key strings.
+constexpr std::string_view hash_key_url = "url";
+constexpr std::string_view hash_key_hostname = "hostname";
+constexpr std::string_view hash_key_path = "path";
+constexpr std::string_view hash_key_path_query = "path+query";
+constexpr std::string_view hash_key_path_fragment = "path+fragment";
+constexpr std::string_view hash_key_cache = "cache_key";
+
+static HostRecord *
+chash_lookup(std::shared_ptr<ATSConsistentHash> ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
Review comment:
This was moved/copied from core. Not sure what you mean? It takes a `ATSConsistentHashIter`, that's what that is. You mean taking all these args and wrapping them up in an iterator object, so things would call `chIter->begin()`, `++chIter`, etc?
----------------------------------------------------------------
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] [trafficserver] a-canary commented on pull request #7023: Add nexthop strategy plugins
Posted by GitBox <gi...@apache.org>.
a-canary commented on pull request #7023:
URL: https://github.com/apache/trafficserver/pull/7023#issuecomment-705798874
-0, AFAIK the general plugin design is to use event handlers. Which I realized will take a lot more work but also simplify the API and the HttpSM quite a bit in the end. I hope we can move towards that once Susan gets the ConnectionSM refactored.
----------------------------------------------------------------
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