Skip to content

Conversation

@brbzull0
Copy link
Contributor

@brbzull0 brbzull0 commented Jan 28, 2022

As a part of the implementation of the JSONRPC mechanism in ATS(part of 10-Dev branch already),traffic_manager became obsolete. This PR holds all the changes related to the removal of traffic_manager and it's related code. This code is being running in Yahoo prod boxes for quite some time already.

TLDR-

  • Removes traffic_manager code and all the legacy RPC mechanisms.
  • Update docs to reflect this change.
  • Remove some mgmt api that depends on traffic_manager to work.
  • Merge some duplicated folders

The Change

A summary of what is being removed and some notes:

  • Management Alarm/Event mechanism removed.
  • TSMgmtAPI
    • The rule here was, if there is a JSONRPC replacement for the API, the code was removed, if the Mgmt api does not uses any of the legacy mgmt mechanism then it was left untouched, like Data Carrier Structures:
      • TSListCreate
      • TSListDestroy
      • TSListEnqueue
    • API's like TSRecordSet, etc which were supposed to be used by remote(traffic_ctl) and local(traffic_manager) clients are no longer needed as now this API is replaced by the JSONRPC API.
    • TSHostStatus* also removed, JSONRPC available now.
    • TSDeleteFromCache* and all the cache related API inside mgmt api was removed as they used the legacy api(TSRecordGetInt)
  • WebMgmtUtils.cc remains. Contains some handy functions although they aren't used. **Contains unused code**
  • mgmt_fatal/log are now removed as this is no longer supported, they were marked as deprecated at least 1 release ago. Removed now and replaced by Debug diag.
  • traffic_manager binary code is removed.
    • Legacy RPC mechanism, LocalManager, ProcessManager.
      - processserver.sock, mgmtapi.sock and eventapi.sock are removed. Events socket was agreed to be removed in one of the ATS Meetup.
    • Records synchronization mechanism between TS running as client and TM running as server is also removed, this was used to sync changes coming in through traffic_manager and then reflected by traffic_server. Stats to disk synchronization left untouched.
  • AdminClient perl module is removed. There are still some extensions which I still need to figure out if it needs to be removed(probably in another PR). Config.pm will not be removed
  • Unit Test
    • Adjust trafficserver.ext to include the full path in the traffic_server cmd so it can be used by different tools to differentiate TS instances.
  • proxy.config.proxy_binary will not work anymore, as this was used by TM to start TS.

Few more things to consider:

  • While working on the jsonrpc I created a mgmt2 folder which was used as a fresh start for things related to management, the idea was at some point after a cleanup of the mgmt folder we can merge mgmt2 folder into mgmt, this change is also included in here.
  • To support JSONRPC there were two folder, traffic_ctl and traffic_ctl_jsonrpc to contain the legacy version and the new version respectively, now the new version overrides the legacy version, so you may see changes in the traffic_ctl code which are actually in 10-Dev branch, this is mainly because of the moving files from one folder to another.
  • I’ve added a note in the documentation warning readers that traffic_manager is no longer supported, this is mostly around records which may find warnings while starting ATS without the records being registered.

The goal beside removing the code is to know exactly what we want to maintain once traffic_manager is gone, questions like, do we want to maintain TSMgmt* API compatibility call the JSONRPC node instead??? Is it worth it? should we keep the API stub and add a warning, so anyone actually using this MGMT Apis gets a warning instead of a build error(missing function)?

Thanks.

Closes #5563

@brbzull0 brbzull0 self-assigned this Jan 28, 2022
@brbzull0 brbzull0 added Documentation Incompatible JSONRPC JSONRPC 2.0 related work. labels Jan 28, 2022
@brbzull0 brbzull0 added this to the 10.0.0 milestone Jan 28, 2022
@brbzull0 brbzull0 force-pushed the dmeden/traffic_manager_removal branch from 7930b8c to d639ffe Compare January 28, 2022 12:12
@brbzull0 brbzull0 marked this pull request as draft January 28, 2022 13:47
@brbzull0 brbzull0 force-pushed the dmeden/traffic_manager_removal branch from d639ffe to 31bb3a8 Compare January 28, 2022 14:03
@brbzull0 brbzull0 marked this pull request as ready for review January 28, 2022 15:29
@brbzull0 brbzull0 requested a review from mlibbey as a code owner January 28, 2022 15:29
@brbzull0
Copy link
Contributor Author

[approve ci autest]

@bryancall
Copy link
Contributor

@SolidWallOfCode is going to look at this

@@ -55,8 +55,6 @@ static const RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.proxy_binary", RECD_STRING, "traffic_server", RECU_NULL, RR_REQUIRED, RECC_NULL, nullptr, RECA_NULL}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't work anymore. It was used by TM.

Copy link
Member

Choose a reason for hiding this comment

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

So, KIWF.

AC_ARG_VAR([DOXYGEN], [full path of Doxygen executable])
AC_ARG_VAR([PERL], [full path of Perl executable])

# Check if MakeMaker is available
Copy link
Member

Choose a reason for hiding this comment

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

Oh, we can finally KIWF "MakeMaker"? Oh frabjous day! Calloo, Callay!

@@ -66,7 +66,6 @@ SLACKWARE="slackware"

function killAll() {
killall traffic_cop
Copy link
Member

Choose a reason for hiding this comment

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

Why is traffic_cop still in here? Didn't that get KIWF'd last version?

@brbzull0 brbzull0 marked this pull request as draft March 15, 2022 11:34
@brbzull0 brbzull0 force-pushed the dmeden/traffic_manager_removal branch 2 times, most recently from 30cdae6 to e131cbd Compare June 1, 2022 18:50
@brbzull0 brbzull0 force-pushed the dmeden/traffic_manager_removal branch 5 times, most recently from 6321b03 to da97f38 Compare June 28, 2022 16:06
@brbzull0 brbzull0 force-pushed the dmeden/traffic_manager_removal branch from da97f38 to 044a1cf Compare June 29, 2022 11:38
@brbzull0 brbzull0 force-pushed the dmeden/traffic_manager_removal branch from 044a1cf to 70d76c3 Compare July 25, 2022 17:41
@ezelkow1
Copy link
Member

[approve ci autest]

1 similar comment
@brbzull0
Copy link
Contributor Author

[approve ci autest]

@brbzull0 brbzull0 force-pushed the dmeden/traffic_manager_removal branch from 56f3431 to 0842f25 Compare July 28, 2022 15:41
@brbzull0
Copy link
Contributor Author

brbzull0 commented Aug 2, 2022

[approve ci autest]

@brbzull0 brbzull0 marked this pull request as ready for review August 2, 2022 15:15
brbzull0 and others added 5 commits August 11, 2022 10:57
traffic_manager binary.
records syncronization between TS running as client and TM running as server.
deprecated functions, ie: mgmt_log
legacy RPC mechanism, LocalManager, ProcessManager.

Remove usage of traffic_manager from some unit tests. I had to make some
adjustments to the way some of the tests "when" clauses were used. TS ready
now uses the default “when” clause(FileContains) which is also checking the
 existence of the file, the latest is what was used in this test.

TM removal: Remove some records used by TM and note in the docs that they are now deprecated.

TM removal: Work the docs so they reflect that TM is no longer supported. This commit
also includes some changes in the Records config file.

TM Removal: Rename new traffic_ctl to the new folder(same as before). Ammend makefiles
to just build the new version.

TM: Removal. Merge mgmt2 and mgmt folders. Use the best from each folder and compile them into a single mgmt folder.
mgmt2 was used to keep track of all new RPC code including things that can be reuse from the old mgmt folder, now with
the clean up for the original folder we can just put the leftovers together.
This also add some notes into some documentation.

TM Removal: Remove Signal,Event and Alarms header files. This is a second part of the removal of the code related to events and alarms.

TM Removal: remove Admin perl module.

TM removal: Fix doc issues and missing include. Fix build issue

Fix not literal string issue
@brbzull0 brbzull0 force-pushed the dmeden/traffic_manager_removal branch from 0842f25 to 56f7aff Compare August 11, 2022 13:58
@brbzull0 brbzull0 merged commit 8524cda into apache:10-Dev Aug 11, 2022
bneradt added a commit to bneradt/trafficserver that referenced this pull request Nov 14, 2022
mgmt/rpc/overridable_txn_vars.cc is a generated file that our .gitignore
should ignore. This used to be in mgmt2, but after apache#8633 it is now in
the mgmt directory. This updates the .gitignore file for the new
location.
bneradt added a commit that referenced this pull request Nov 14, 2022
mgmt/rpc/overridable_txn_vars.cc is a generated file that our .gitignore
should ignore. This used to be in mgmt2, but after #8633 it is now in
the mgmt directory. This updates the .gitignore file for the new
location.
@bryancall bryancall mentioned this pull request Aug 14, 2024
91 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants