You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Igor Galić <i....@brainsware.org> on 2013/07/19 12:49:33 UTC
Re: git commit: TS-1487: Fix startup order, add lifecycle hooks
----- Original Message -----
> Updated Branches:
> refs/heads/master ea560cecc -> 941784b2a
>
>
> TS-1487: Fix startup order, add lifecycle hooks
vs
> ----------------------------------------------------------------------
> diff --git a/CHANGES b/CHANGES
> index 599f7bf..d6889d8 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -1,6 +1,8 @@
> -*- coding:
> utf-8 -*-
> Changes with Apache Traffic Server 3.3.5
>
> + *) [TS-1487] [TS-2035] Moved plugin init, added plugin lifecycle
> hooks, added delay listen for cache. Removed TS_NO_API defined/build
> option.
> +
Why are those three changes all munged into one commit?
> *) [TS-2047] Schedule RamCacheCLFUSCompressor in
> RamCacheCLFUS::init instead
> of immediatly after instantiation.
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/configure.ac
> ----------------------------------------------------------------------
> diff --git a/configure.ac b/configure.ac
> index 0bc7a00..0d2285a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -279,19 +279,6 @@ AC_MSG_RESULT([$enable_remote_cov_commit])
> AC_SUBST([enable_remote_cov_commit])
>
> #
> -# API
> -#
> -AC_MSG_CHECKING([whether to enable API and plugin support])
> -AC_ARG_ENABLE([api],
> - [AS_HELP_STRING([--disable-api],[do not enable API and plugin
> support])],
> - [],
> - [enable_api=yes]
> -)
> -AC_MSG_RESULT([$enable_api])
> -AS_IF([test "x$enable_api" = "xyes"], [has_inkapi=1],
> [has_inkapi=0])
> -AC_SUBST(has_inkapi)
> -
> -#
> # WCCP
> #
> AC_MSG_CHECKING([whether to enable WCCP v2 support])
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/example/Makefile.am
> ----------------------------------------------------------------------
> diff --git a/example/Makefile.am b/example/Makefile.am
> index e0939e0..193e1c4 100644
> --- a/example/Makefile.am
> +++ b/example/Makefile.am
> @@ -28,6 +28,7 @@ noinst_LTLIBRARIES = \
> cache-scan.la \
> file-1.la \
> hello.la \
> + lifecycle-plugin.la \
tab vs spaces
> null-transform.la \
> output-header.la \
> protocol.la \
> @@ -48,6 +49,7 @@ bnull_transform_la_SOURCES =
> bnull-transform/bnull-transform.c
> cache_scan_la_SOURCES = cache-scan/cache-scan.cc
> file_1_la_SOURCES = file-1/file-1.c
> hello_la_SOURCES = hello/hello.c
> +lifecycle_plugin_la_SOURCES = lifecycle-plugin/lifecycle-plugin.c
> null_transform_la_SOURCES = null-transform/null-transform.c
> output_header_la_SOURCES = output-header/output-header.c
> protocol_la_SOURCES = protocol/Protocol.c protocol/TxnSM.c
>
[snip]
> ----------------------------------------------------------------------
> diff --git a/example/lifecycle-plugin/lifecycle-plugin.c
> b/example/lifecycle-plugin/lifecycle-plugin.c
> new file mode 100644
> index 0000000..e08fbc3
> --- /dev/null
> +++ b/example/lifecycle-plugin/lifecycle-plugin.c
[snip]
> +int
> +CheckVersion()
> +{
> + const char *ts_version = TSTrafficServerVersionGet();
> + int result = 0;
> +
> + if (ts_version) {
> + int major_ts_version = 0;
> + int minor_ts_version = 0;
> + int patch_ts_version = 0;
> +
> + if (sscanf(ts_version, "%d.%d.%d", &major_ts_version,
> &minor_ts_version, &patch_ts_version) != 3) {
> + return 0;
> + }
> +
> + /* Need at least TS 3.3.5 */
> + if (major_ts_version > 3 ||
> + (major_ts_version == 3 &&
> + (minor_ts_version > 3 ||
> + (minor_ts_version == 3 && patch_ts_version >= 5)))) {
> + result = 1;
> + }
> + }
> + return result;
> +}
another reminder of https://issues.apache.org/jira/browse/TS-1953
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/mgmt/RecordsConfig.cc
> ----------------------------------------------------------------------
> diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc
> index f0cc3c8..66102f4 100644
> --- a/mgmt/RecordsConfig.cc
> +++ b/mgmt/RecordsConfig.cc
> @@ -390,6 +390,8 @@ RecordElement RecordsConfig[] = {
> ,
> {RECT_CONFIG, "proxy.config.http.server_other_ports", RECD_STRING,
> NULL, RECU_RESTART_TM, RR_NULL, RECC_NULL, NULL, RECA_NULL}
> ,
> + {RECT_CONFIG, "proxy.config.http.wait_for_cache", RECD_INT, "1",
> RECU_RESTART_TM, RR_NULL, RECC_INT, "[0-1]", RECA_NULL}
> + ,
maybe in CHANGES we should write what the consequence of that change
was rather than writing that we've made some change to fix TS-blah-blah
> {RECT_CONFIG, "proxy.config.http.insert_request_via_str",
> RECD_INT, "1", RECU_DYNAMIC, RR_NULL, RECC_NULL, NULL, RECA_NULL}
> ,
> {RECT_CONFIG, "proxy.config.http.insert_response_via_str",
> RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_NULL, NULL, RECA_NULL}
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/proxy/IPAllow.h
> ----------------------------------------------------------------------
> diff --git a/proxy/IPAllow.h b/proxy/IPAllow.h
> index 941a7e7..76ecdf4 100644
> --- a/proxy/IPAllow.h
> +++ b/proxy/IPAllow.h
> @@ -34,7 +34,6 @@
> #include "Main.h"
> #include "hdrs/HTTP.h"
> #include "ts/IpMap.h"
> -#include "vector"
> #include "ts/Vec.h"
> #include "ProxyConfig.h"
This seems to be part of an unrelated(?) clean-up change.
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/proxy/InkAPI.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/InkAPI.cc b/proxy/InkAPI.cc
> index cabab42..540bec8 100644
> --- a/proxy/InkAPI.cc
> +++ b/proxy/InkAPI.cc
> @@ -21,8 +21,6 @@
> limitations under the License.
> */
>
> -#ifndef TS_NO_API
> -
> // Avoid complaining about the deprecated APIs.
> // #define TS_DEPRECATED
>
> @@ -369,6 +367,7 @@ tsapi const char * TS_NPN_PROTOCOL_SPDY_3 =
> "spdy/3"; // upcoming
> tsapi const TSMLoc TS_NULL_MLOC = (TSMLoc)NULL;
>
> HttpAPIHooks *http_global_hooks = NULL;
> +LifecycleAPIHooks* lifecycle_hooks = NULL;
> ConfigUpdateCbTable *global_config_cbs = NULL;
>
> static char traffic_server_version[128] = "";
> @@ -630,6 +629,13 @@ sdk_sanity_check_hook_id(TSHttpHookID id)
> return TS_SUCCESS;
> }
>
> +TSReturnCode
> +sdk_sanity_check_lifecycle_hook_id(TSLifecycleHookID id)
> +{
> + if (id<TS_LIFECYCLE_PORTS_INITIALIZED_HOOK || id> TS_LIFECYCLE_LAST_HOOK)
please try to use consistent spacing for </>, see below:
[wild-snipping]
> +template < typename ID, ID MAX_ID >
> +FeatureAPIHooks<ID,MAX_ID>::FeatureAPIHooks():
> +template < typename ID, ID MAX_ID >
> +FeatureAPIHooks<ID,MAX_ID>::~FeatureAPIHooks()
> +template < typename ID, ID MAX_ID >
> +void
> +FeatureAPIHooks<ID,MAX_ID>::clear()
> +template < typename ID, ID MAX_ID >
> +FeatureAPIHooks<ID,MAX_ID>::prepend(ID id, INKContInternal *cont)
> +template < typename ID, ID MAX_ID >
> +void
> +FeatureAPIHooks<ID,MAX_ID>::append(ID id, INKContInternal *cont)
> +template < typename ID, ID MAX_ID >
> +APIHook *
> +FeatureAPIHooks<ID,MAX_ID>::get(ID id)
> +template < typename ID, ID MAX_ID >
> +bool
> +FeatureAPIHooks<ID,MAX_ID>::has_hooks() const
> +class HttpAPIHooks : public FeatureAPIHooks<TSHttpHookID, TS_HTTP_LAST_HOOK>
> +class LifecycleAPIHooks : public FeatureAPIHooks<TSLifecycleHookID, TS_LIFECYCLE_LAST_HOOK>
[/wild-snipping]
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/proxy/api/ts/ts.h.in
> ----------------------------------------------------------------------
> diff --git a/proxy/api/ts/ts.h.in b/proxy/api/ts/ts.h.in
> index 5ba4ebe..fc65e0e 100644
> --- a/proxy/api/ts/ts.h.in
> +++ b/proxy/api/ts/ts.h.in
> @@ -276,6 +276,55 @@ extern "C"
> } TSHttpHookID;
> #define TS_HTTP_READ_REQUEST_PRE_REMAP_HOOK TS_HTTP_PRE_REMAP_HOOK
> /* backwards compat */
>
> + /** Plugin lifecycle hooks.
> +
> + These are called during lifecycle events of a plugin. They
> + should be set in the plugin initialization function. The
> + continuation is invoked with an event ID specified for each
> hook
> + and @c NULL for the event data.
> +
> + TS_LIFECYCLE_PORTS_INITIALIZED_HOOK
> +
> + called once, after the HTTP proxy port data structures have
> + been initialized. In particular, SSL related calls that
> depend
> + on accept endpoints may be invoked. After this hook is
> + finished, the proxy port sockets are opened and connections
> + are accepted.
> +
> + Event: TS_EVENT_LIFECYCLE_PORTS_INITIALIZED
> +
> + TS_LIFECYCLE_PORTS_READY_HOOK
> +
> + called once, after the sockets have been opened and the
> accept
> + threads have been started. That is, the ports are ready to
> + accept connections. This is *not* guaranteed to be called
> + before the first connection is accepted.
> +
> + Event: TS_EVENT_LIFECYCLE_PORTS_READY_HOOK
> +
> + TS_LIFECYCLE_CACHE_READY_HOOK
> +
> + called once, after the cache has finished its
> + initialization. It is either online or has failed when this
> + hook is called.
> +
> + Event: TS_EVENT_LIFECYCLE_CACHE_READY
> +
> + Ordering guarantees:
> +
> + - TS_LIFECYCLE_PORTS_INITIALIZED_HOOK before
> TS_LIFECYCLE_PORTS_READY_HOOK.
> +
> + NOTE! ONLY the orderings EXPLICITLY mentioned above are
> guaranteed.
> +
> + */
> + typedef enum
> + {
> + TS_LIFECYCLE_PORTS_INITIALIZED_HOOK,
> + TS_LIFECYCLE_PORTS_READY_HOOK,
> + TS_LIFECYCLE_CACHE_READY_HOOK,
> + TS_LIFECYCLE_LAST_HOOK
> + } TSLifecycleHookID;
> +
Could you, following jpeach's example add a man page for this API?
[snip]
> --- a/proxy/http/HttpProxyServerMain.cc
> +++ b/proxy/http/HttpProxyServerMain.cc
> @@ -39,7 +39,6 @@
> HttpAccept *plugin_http_accept = NULL;
> HttpAccept *plugin_http_transparent_accept = 0;
>
> -#if !defined(TS_NO_API)
> static SLL<SSLNextProtocolAccept> ssl_plugin_acceptors;
> static ProcessMutex ssl_plugin_mutex;
>
> @@ -73,47 +72,43 @@ ssl_unregister_protocol(const char * protocol,
> Continuation * contp)
> return true;
> }
>
> -#endif /* !defined(TS_NO_API) */
> -
> /////////////////////////////////////////////////////////////////
> //
> // main()
> //
> /////////////////////////////////////////////////////////////////
> -void
> -init_HttpProxyServer(void)
> -{
> -#ifndef INK_NO_REVERSE
> - init_reverse_proxy();
> -#endif
reminder: https://issues.apache.org/jira/browse/TS-819
https://issues.apache.org/jira/browse/TS-1685
[snip]
>
> - // XXX although we make a good pretence here, I don't believe that
> NetProcessor::main_accept() ever actually returns
> - // NULL. It would be useful to be able to detect errors and spew
> them here though.
hah...
> + // Do the configuration defined ports.
> + for ( int i = 0 , n = proxy_ports.length() ; i < n ; ++i ) {
> + MakeHttpProxyAcceptor(HttpProxyAcceptors.add(), proxy_ports[i],
> n_accept_threads);
> + }
> +
> }
>
> void
> -start_HttpProxyServer(int accept_threads)
> +start_HttpProxyServer()
> {
> static bool called_once = false;
> + HttpProxyPort::Group& proxy_ports = HttpProxyPort::global();
>
> ///////////////////////////////////
> // start accepting connections //
> ///////////////////////////////////
>
> ink_assert(!called_once);
> -
> - for ( int i = 0 , n = HttpProxyPort::global().length() ; i < n ;
> ++i ) {
> - start_HttpProxyPort(HttpProxyPort::global()[i], accept_threads);
> + ink_assert(proxy_ports.length() == HttpProxyAcceptors.length());
> +
> + for ( int i = 0 , n = proxy_ports.length() ; i < n ; ++i ) {
> + HttpProxyAcceptor& acceptor = HttpProxyAcceptors[i];
> + HttpProxyPort& port = proxy_ports[i];
> + if (port.isSSL()) {
> + if (NULL == sslNetProcessor.main_accept(acceptor._accept,
> port.m_fd, acceptor._net_opt))
> + return;
> + } else {
> + if (NULL == netProcessor.main_accept(acceptor._accept,
> port.m_fd, acceptor._net_opt))
> + return;
> + }
> + // XXX although we make a good pretence here, I don't believe
> that NetProcessor::main_accept() ever actually returns
> + // NULL. It would be useful to be able to detect errors and spew
> them here though.
oh.. I thought you had that fixed :\
> }
>
> #if TS_HAS_TESTS
> @@ -204,6 +248,14 @@ start_HttpProxyServer(int accept_threads)
> init_http_update_test();
> }
> #endif
> +
> + // Alert plugins that connections will be accepted.
> + APIHook* hook =
> lifecycle_hooks->get(TS_LIFECYCLE_PORTS_READY_HOOK);
> + while (hook) {
> + hook->invoke(TS_EVENT_LIFECYCLE_PORTS_READY, NULL);
> + hook = hook->next();
> + }
> +
> }
>
> void
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/proxy/http/HttpProxyServerMain.h
> ----------------------------------------------------------------------
> diff --git a/proxy/http/HttpProxyServerMain.h
> b/proxy/http/HttpProxyServerMain.h
> index 1d0502e..98769ff 100644
> --- a/proxy/http/HttpProxyServerMain.h
> +++ b/proxy/http/HttpProxyServerMain.h
> @@ -23,12 +23,14 @@
>
> struct HttpProxyPort;
>
> -void init_HttpProxyServer(void);
> +/** Initialize all HTTP proxy port data structures needed to run.
> + */
> +void init_HttpProxyServer(int n_accept_threads = 0);
>
> /** Start the proxy server.
> - The ports are contained in the HttpProxyPort global data.
> + The port data should have been created by @c
> init_HttpProxyServer().
> */
> -void start_HttpProxyServer(int accept_threads = 0);
> +void start_HttpProxyServer();
>
> void start_HttpProxyServerBackDoor(int port, int accept_threads =
> 0);
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/proxy/http/HttpSM.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
> index 1ebf842..4cfe0d1 100644
> --- a/proxy/http/HttpSM.cc
> +++ b/proxy/http/HttpSM.cc
> @@ -612,7 +612,7 @@ HttpSM::attach_client_session(HttpClientSession *
> client_vc, IOBufferReader * bu
> HTTP_INCREMENT_DYN_STAT(http_current_client_transactions_stat);
>
> // Record api hook set state
> - hooks_set = http_global_hooks->hooks_set | client_vc->hooks_set;
> + hooks_set = http_global_hooks->has_hooks() ||
> client_vc->hooks_set;
>
> // Setup for parsing the header
> ua_buffer_reader = buffer_reader;
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/proxy/http/HttpTransact.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
> index 820663a..c8cc90f 100644
> --- a/proxy/http/HttpTransact.cc
> +++ b/proxy/http/HttpTransact.cc
> @@ -4617,7 +4617,6 @@ HttpTransact::handle_transform_ready(State* s)
> void
> HttpTransact::set_header_for_transform(State* s, HTTPHdr*
> base_header)
> {
> -#ifndef TS_NO_TRANSFORM
> s->hdr_info.transform_response.create(HTTP_TYPE_RESPONSE);
> s->hdr_info.transform_response.copy(base_header);
>
> @@ -4628,9 +4627,6 @@ HttpTransact::set_header_for_transform(State*
> s, HTTPHdr* base_header)
>
> if (!s->cop_test_page)
> DUMP_HEADER("http_hdrs", &s->hdr_info.transform_response,
> s->state_machine_id, "Header To Transform");
> -#else
> - ink_assert(!"transformation not supported\n");
> -#endif // TS_NO_TRANSFORM
> }
>
> void
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/941784b2/proxy/http/HttpUpdateSM.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/http/HttpUpdateSM.cc b/proxy/http/HttpUpdateSM.cc
> index 7b5eddf..6b47881 100644
> --- a/proxy/http/HttpUpdateSM.cc
> +++ b/proxy/http/HttpUpdateSM.cc
> @@ -116,7 +116,6 @@ HttpUpdateSM::handle_api_return()
> }
>
> switch (t_state.next_action) {
> -#ifndef TS_NO_TRANSFORM
> case HttpTransact::TRANSFORM_READ:
> {
> if (t_state.cache_info.transform_action ==
> HttpTransact::CACHE_DO_WRITE) {
> @@ -154,7 +153,6 @@ HttpUpdateSM::handle_api_return()
> }
> break;
> }
> -#endif //TS_NO_TRANSFORM
> case HttpTransact::PROXY_INTERNAL_CACHE_WRITE:
> case HttpTransact::SERVER_READ:
> case HttpTransact::PROXY_INTERNAL_CACHE_NOOP:
>
only now do I realize that you've not just been removing
TS_NO_API, but also TS_NO_TRANSFORM from the code as well..
--
Igor Galić
Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515 2EA5 4B1D 9E08 A097 C9AE
Re: git commit: TS-1487: Fix startup order, add lifecycle hooks
Posted by "Alan M. Carroll" <am...@network-geographics.com>.
Friday, July 19, 2013, 5:49:33 AM, you wrote:
>> Changes with Apache Traffic Server 3.3.5
>>
>> + *) [TS-1487] [TS-2035] Moved plugin init, added plugin lifecycle
>> hooks, added delay listen for cache. Removed TS_NO_API defined/build
>> option.
>> +
> Why are those three changes all munged into one commit?
Because TS-2035 is a duplicate of TS-1487 and the rest of that is part of the proposed fix for TS-1487. The TS_NO_API is in there because it was getting in my way while I was fixing this and I solved it zwoop-style.